classification
Title: atexit module not safe in Python 3.0 with multiple interpreters
Type: behavior Stage:
Components: Interpreter Core Versions: Python 3.0, Python 3.1
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: loewis Nosy List: barry, benjamin.peterson, christian.heimes, grahamd, loewis
Priority: release blocker Keywords: needs review, patch

Created on 2008-10-24 22:45 by grahamd, last changed 2009-07-21 11:20 by grahamd. This issue is now closed.

Files
File name Uploaded Description Edit
atexit_modulestate.patch christian.heimes, 2008-10-26 20:15
Messages (18)
msg75195 - (view) Author: Graham Dumpleton (grahamd) Date: 2008-10-24 22:45
In Python 3.0 the atexit module was translated from Python code to C code.

Prior to Python 3.0, because it was implemented at C code level, the list of 
callbacks was specific to each sub interpreter. In Python 3.0 that appears to 
no longer be the case, with the list of callbacks being maintained globally as 
static C variable across all interpreters.

The end result is that if a sub interpreter registers an atexit callback, that 
callback will be called within the context of the main interpreter on call of 
Py_Finalize(), and not in context of the sub interpreter against which it was 
registered.

Various problems could ensue from this depending on whether or not the sub 
interpreter had also since been destroyed.

Still need to validate the above, but from visual inspection looks to be a 
problem. Issue found because mod_wsgi will trigger atexit callbacks for a sub 
interpreter when the sub interpreter is being destroyed. This is because Python 
prior to 3.0 only did it from the main interpreter. In Python 3.0, this all 
seems to get messed up now as no isolation between callbacks for each 
interpreter.

For mod_wsgi case, since it is explicitly triggering atexit callbacks for sub 
interpreter, in doing so it is actually calling all registered atexit callbacks 
across all interpreters for each sub interpreter being destroyed. They then 
again get called for Py_Finalize().

Even if mod_wsgi weren't doing this, still a problem that Py_Finalize() calling 
atexit callbacks in context of main interpreter which were actually associated 
with sub interpreter.

For mod_wsgi, will probably end up installing own 'atexit' module variant in 
sub interpreters to ensure separation.
msg75199 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-10-25 00:57
Ouch! We totally forgot about sub interpreters during the
reimplementation of the atexit module. 

In my opinion it's a big bug for systems like mod_python and mod_wsgi.
However it's too late to fix it for 3.0.0. Perhaps we can fix it in
3.0.1. We always knew that Python 3.0 is going to have some bugs that
can only be revealed by 3rd party code. I've added Barry to the nosy
list in order to get his opinion as release manager. 

Do you have a suggestion how the problem should be solved? I'd store the
atexit_callback information in PyInterpreterState instead of the module.
Maybe Martin's PEP 3121 could come into play to rescue us.

By the way PEP 3121 ... does the new module initialization code play
nice with multiple sub interpreters?
msg75200 - (view) Author: Graham Dumpleton (grahamd) Date: 2008-10-25 02:40
I wouldn't be concerned about mod_python as likelihood that it will be ported is 
very low and even if it was it would be a long long way in the future. There is too 
much in mod_python itself unrelated to Python 3.0 that needs to be fixed before a 
port to Python 3.0 should be considered.

As to mod_wsgi, I can work around it in the short term by installing an 'atexit' 
module replacement in sub interpreters and avoid the problem.

As to a solution, yes, using PyInterpreterState would seem the most logical place, 
however there is a lot more to it than that.

Prior to Python 3.0, any callbacks registered with atexit module in sub interpreters 
weren't called anyway. This is because Py_EndInterpreter() didn't trigger them, nor 
did Py_Finalize(). The latter is in Python 3.0 at the moment, but as pointed out 
that is a problem in itself.

So, although one may register sub interpreter atexit callbacks against PyInterpreterState, what would be done with them. A decision would need to be made 
as to whether Py_EndInterpreter() should trigger them, or whether the status quo be 
maintained and nothing done with them.

In the short term, ie., for Python 3.0.0, the simplest thing to do may be to have 
functions of atexit module silently not actually do anything for sub interpreters.

The only place this would probably cause a problem would be for mod_wsgi where it 
was itself calling sys.exitfunc() on sub interpreters to ensure they were run. Since 
mod_wsgi has to change for Python 3.0 anyway, to call atexit._run_exitfuncs, with a 
bit more work mod_wsgi can just replace atexit module altogether in sub interpreter 
context and have mod_wsgi track the callback functions and trigger them.

By having atexit module ignore stuff for sub interpreters, at least for now avoid 
problem of callbacks against sub interpreters being execute by Py_Finalize() in main 
interpreter context.

And no I haven't looked at how PEP 3121 has changed things in Python 3.0. Up till 
now I hadn't seen any problems to suggest I may need to look at it.
msg75232 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-10-26 15:42
I'm adding Martin to the nosy list as well. He is the author of PEP 3121
and he might be able to give more insight in the problem.
msg75239 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-10-26 18:36
It would certainly be possible to produce per-interpreter callback lists
through the module state. You declare a per-interpreter structure, add
its size into atexitmodule, and refer to it through PyModule_GetDef,
passing the self argument of the module-level functions.

Reviewing the code, I have two unrelated remarks:
- METH_NOARGS functions still take two arguments.
- I would drop the double-indirection of the callback list. Instead,
allocate a single block of atexit_callback, and use func==NULL as an
indicator that the block is free.

According to the documentation of the atexit module, it seems clear that
it is a per-interpreter thing.
msg75241 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-10-26 18:56
I'm trying to come up with a patch. It's a little bit trickier than I
thought. The atexit module registers "atexit_callfuncs()" with
_Py_PyAtExit(). The "atexit_callfuncs()" function is called by
Python/pythonrun.c when the interpreter exits. The function is cleared
as "static void atexit_callfuncs(void)" so it doesn't get the module
through self. However PyModule_GetDef requires self.

In order to use PEP 3121 we have to add a mechanism to pass the module
instance to atexit_callfuncs().
msg75243 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-10-26 19:18
Christian Heimes wrote:
> However PyModule_GetDef requires self.

(I actually meant PyModule_GetState).

You can use PyState_FindModule to quickly lookup a module.
msg75244 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-10-26 19:26
Ah, thanks. I'll try PyState_FindModule().
msg75246 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-10-26 20:15
Can you review the patch, Martin? I've moved the three state variables
into a module state struct. The patch also fixes the METH_NOARGS functions.
msg75278 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-10-28 11:22
Martin, can you please review and comment on my patch?

Graham, does the patch solve your problem?
msg75279 - (view) Author: Graham Dumpleton (grahamd) Date: 2008-10-28 11:27
By visual inspection the intent looks correct, but can't actually test it 
until I can checkout Python code from source repository and apply patch as 
patch doesn't apply cleanly to 3.0rc1.

With #3723 and #4213 now also having patches, will need to sit down and 
look at all of them and see if find any new issues. May take me a couple 
of days to get time.
msg75309 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-10-28 22:54
This patch looks fine.
msg75339 - (view) Author: Graham Dumpleton (grahamd) Date: 2008-10-30 00:11
In conjunction with #3723 and #4213, the attached atexit_modulestate.patch 
appears to fix issue for mod_wsgi.
msg75383 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2008-10-30 21:34
Fixed in r67054
msg90710 - (view) Author: Graham Dumpleton (grahamd) Date: 2009-07-19 12:13
I know this issue is closed, but for this patch, the code:

+    modstate = get_atexitmodule_state(module);
+
+    if (modstate->ncallbacks == 0)
+        return;

was added.

Is there any condition under which modstate could be NULL.

Haven't touched Python 3.0 support in mod_wsgi for a long time and when 
revisiting code with final Python 3.0, I find that I get Py_Finalize() 
crashing on process shutdown. It is crashing because modstate above is 
NULL.
msg90713 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-07-19 16:33
2009/7/19 Graham Dumpleton <report@bugs.python.org>:
>
> Graham Dumpleton <Graham.Dumpleton@gmail.com> added the comment:
>
> I know this issue is closed, but for this patch, the code:
>
> +    modstate = get_atexitmodule_state(module);
> +
> +    if (modstate->ncallbacks == 0)
> +        return;
>
> was added.
>
> Is there any condition under which modstate could be NULL.

The module is probably being deallocated before the callbacks are being called.

>
> Haven't touched Python 3.0 support in mod_wsgi for a long time and when
> revisiting code with final Python 3.0, I find that I get Py_Finalize()
> crashing on process shutdown. It is crashing because modstate above is
> NULL.
msg90731 - (view) Author: Graham Dumpleton (grahamd) Date: 2009-07-20 12:42
This new problem I am seeing looks like it may be linked to where the 
'atexit' module is initialised/imported in a sub interpreter but never 
in the main interpreter. I can avoid the crash by having:

    PyImport_ImportModule("atexit");

    Py_Finalize();

At a guess, this is because:

    module = PyState_FindModule(&atexitmodule);
    if (module == NULL)
        return;

still returns a module for case where imported in a sub interpreter but 
not in main interpreter, but then:

    modstate = GET_ATEXIT_STATE(module);

    if (modstate->ncallbacks == 0)
        return;

returns NULL for modstate for the main interpreter as PyInit_atexit() 
had never been called for the main interpreter.

The fix would appear to be to check modstate for being NULL and return. 
Ie.,

    module = PyState_FindModule(&atexitmodule);
    if (module == NULL)
        return;
    modstate = GET_ATEXIT_STATE(module);

    if (modstate == NULL)
        return;

    if (modstate->ncallbacks == 0)
        return;

Does that make sense to anyone? If it does and I am correct, I'll create 
a new issue for it as original fix seems deficient.
msg90754 - (view) Author: Graham Dumpleton (grahamd) Date: 2009-07-21 11:20
Have created issue6531 for my new issue related to this patch.
History
Date User Action Args
2009-07-21 11:20:10grahamdsetmessages: + msg90754
2009-07-20 12:42:51grahamdsetmessages: + msg90731
2009-07-19 16:33:21benjamin.petersonsetmessages: + msg90713
2009-07-19 12:13:48grahamdsetmessages: + msg90710
2008-10-30 21:34:24christian.heimessetstatus: open -> closed
resolution: fixed
messages: + msg75383
2008-10-30 00:11:16grahamdsetmessages: + msg75339
2008-10-28 22:54:12benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg75309
2008-10-28 11:27:12grahamdsetmessages: + msg75279
2008-10-28 11:22:48christian.heimessetassignee: loewis
messages: + msg75278
2008-10-26 20:58:24christian.heimessetpriority: critical -> release blocker
2008-10-26 20:15:51christian.heimessetkeywords: + needs review, patch
files: + atexit_modulestate.patch
messages: + msg75246
2008-10-26 19:26:26christian.heimessetmessages: + msg75244
2008-10-26 19:18:08loewissetmessages: + msg75243
2008-10-26 18:56:52christian.heimessetmessages: + msg75241
2008-10-26 18:36:12loewissetmessages: + msg75239
2008-10-26 15:42:42christian.heimessetnosy: + loewis
messages: + msg75232
2008-10-25 02:40:43grahamdsetmessages: + msg75200
2008-10-25 00:57:57christian.heimessetpriority: critical
nosy: + barry, christian.heimes
messages: + msg75199
versions: + Python 3.1
2008-10-24 22:45:53grahamdcreate