classification
Title: importing a module that executes fork() raises RuntimeError
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.1, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: Alex.Roitman, barry, brett.cannon, eric.araujo, gregory.p.smith, ncoghlan, r.david.murray
Priority: normal Keywords: patch

Created on 2010-08-11 23:42 by Alex.Roitman, last changed 2010-12-02 04:36 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
issue9573_fork_on_import.diff ncoghlan, 2010-08-17 02:58 Account for import lock nesting in _PyImport_ReInitLock
fork_on_import.py ncoghlan, 2010-08-17 03:01 Test script that works post-patch but fails without it
Messages (20)
msg113643 - (view) Author: Alex Roitman (Alex.Roitman) Date: 2010-08-11 23:42
Importing the module with the following contents results in RuntimeError:

==================
import os
pid = os.fork()

if pid == 0:
    print "In the child"
else:
    print "In the parent"
print "Done\n"
==================

Running the same module as main works just fine, so it seems to be a purely import issue.

I looked into the 2.6.6rc1 source.  This is what I think happens in Python/import.c file:

1. After the fork() call, _PyImport_ReInitLock() is run. It sets import_lock_thread to -1 in the child, line 310.

2. In _PyImport_ReleaseLock() line 290 compares import_loc_thread to the current thread id, and if they are not the same then -1 is returned, which results in RuntimeError in PyImport_ImportModuleLevel (line 2186-2189)

So this is guaranteed to happen in the child, every time fork() is executed inside the module being imported.  If I change line 290 to be:

    if (import_lock_thread != me && import_lock_thread != -1)

then import proceeds fine, although I'm not sure this is a proper solution.

This happens on Linux, Darwin, and Cygwin, with python 2.6.5 and higher. I'd be happy to assist solving this, please let me know how I can help.
msg113648 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-08-12 02:06
This may be a case of "don't do that".  Starting a new thread or process during import (ie: while the import lock is held) is dangerous.  Nosying Brett, since he'll know the real story.
msg113649 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2010-08-12 02:13
Without looking closer at it, don't do that. =)
msg113651 - (view) Author: Alex Roitman (Alex.Roitman) Date: 2010-08-12 02:48
I guess I am missing something here.  In a complex program, everything will be executed in some module or another.  Consequently, the module that contains the fork() call will cause the interpreter to quit.

How can this be worked around, short of placing the fork() in the main module?
msg113652 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-08-12 02:54
> How can this be worked around, short of placing the fork()
> in the main module?

Why wouldn't you place the fork() call in a function?
msg113653 - (view) Author: Alex Roitman (Alex.Roitman) Date: 2010-08-12 02:56
I can place it in a function.  But if I execute that function from anything other than main module, the fork() will be called while import lock is held, one way or another.  It will just happen in another module.  So what?
msg113656 - (view) Author: Alex Roitman (Alex.Roitman) Date: 2010-08-12 05:21
1. If fork should not be called during import, it could raise an exception when invoked from import.  But it does not.  Is that a bug then?  BTW, fork during import worked with python 2.4 just fine.

2. The whole issue7242 was devoted to work out import locks during forking. 5 months ago r78527 was committed to to just that (although it is not perfect). If this is not the proper use case then had it been done in error?

3. belopolsky: Thanks for the advice to use the mailing list.  I'd appreciate it if instead you refrained from publishing my email address on this page.
msg114094 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-08-17 02:08
Added Greg to nosy list as the one that fixed issue 7242 with the current _PyImport_ReInitLock semantics.

Also kicking over to Barry regarding implications for 2.6.6 (this is a regression from 2.6.4 due to the resolution of 7242).

7242 was about forking from a *thread*. This is about forking as a side effect of import, which, just like spawning a thread as a side effect of import, can easily cause issues.

The RuntimeError noted by the OP isn't thrown by the fork() call - it is thrown at the end of the import when control is returned to the main module. Completely reinitialising the lock without accounting for the current depth of nesting is incorrect. Instead, I believe ReInitLock should look more like:

    if (import_lock != NULL)
        import_lock = PyThread_allocate_lock();
    if (import_lock_level > 1) {
        /* Forked as a side effect of import */
        long me = PyThread_get_thread_ident();
        import_lock_thread = me;
        import_lock_level--;
    } else {
        import_lock_thread = -1;
        import_lock_level = 0;
    }

(I haven't tested that yet, but will soon)
msg114095 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-08-17 02:22
One slight tweak to that suggested change - the lock reinitialisation needs to acquire the new lock in the first branch.
msg114097 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-08-17 02:58
Minimal patch attached (no niceties like NEWS or unit tests included yet)
msg114098 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-08-17 03:01
Test script attached that demonstrates the underlying problem directly via imp.lock_held() (this could easily form the basis of a unit test)
msg114143 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2010-08-17 18:43
I think it's too late to try to get this into 2.6.6.  rc2's already been released, I don't expect/want an rc3, and I'm not comfortable changing this at this point.  Unless you can convince me it's absolutely critical, I won't approve this for 2.6.6.
msg114153 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2010-08-17 19:59
Are there any applications out there that actually rely on forking during import?

(someone discovered this bug... i'd like to know why.  i think its a disgusting thing to do but never explicitly disallowed it in the past)
msg114166 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-08-17 20:56
It turns out my proposed patch is incorrect anyway - it will do the wrong thing if a thread *other* than the one doing the fork is in the middle of a nested import at the time the fork occurs.

With issue 7242 establishing that the current thread ID may not survive the forking process on all platforms, the only way I can see to get completely correct semantics for the fork-as-a-side-effect of import case is to give the forking thread another way to detect "did I own the import lock before the fork?". One way to do that would be to move the lock nesting count into thread local storage, although that would likely suffer cross-platform incompatibility fun and games as well.

Given that, I'm inclined to go with what Brett said: don't do that. Use a __name__ == "__main__" guard so the fork only happens when run as a script, not when imported.
msg114168 - (view) Author: Alex Roitman (Alex.Roitman) Date: 2010-08-17 21:07
gregory.p.smith: This is my use case: we had the following situation with the test scripts at work.  Each script is required to import TestApi module in order to run the tests.  That module in turn imported the module that forks, and in the parent waits for the child to exit, then kills all child's children processes.  That way tests don't leave any processes behind.

So any script that imported the cleanup module, whether directly or via another module, had this cleanup functionality "for free". One workaround for this issue would be to change all existing test scripts to call the cleanup function, instead of the cleanup module calling it at the module level.

I can see your reservations about forking/starting threads during import, but it seems like it either should work or it should be disallowed.  The thing is, the actual import is working fine with the fork() call, it's releasing the lock that is messed up, because it was not initialized correctly after the fork.  The patch attached by ncoghlan looks good though.
msg114169 - (view) Author: Alex Roitman (Alex.Roitman) Date: 2010-08-17 21:13
I already worked around this for my use case.

For the future, it would be nice if fork() raised an exception if called during the import, and if the documentation mentioned that forking while in import is not allowed.
msg114171 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2010-08-17 21:18
Agreed on the explicit exception and documentation. :)
msg114173 - (view) Author: Alex Roitman (Alex.Roitman) Date: 2010-08-17 21:22
Will starting a thread while in import also be disallowed?  If so, issue 7242 will also become moot...
msg114174 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-08-17 21:25
On further further reflection - I'm back to thinking my patch is correct. With the way fork is now implemented, the forking thread *always* holds the import lock, so the assumption in my patch regarding the meaning of the nesting level is correct.

It could use a comment in _PyImport_ReInitLock to better explain that, though.

Unless there are any objections, I'll apply the fix to 2.7, 3.1 and 3.2.

2.6 is out of luck though (as per Barry's comment). I'll do a doc change for that, but I can't promise I'll get to it before the binary releases go out.
msg123054 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-12-02 04:36
Fixed for 3.2 in r86928.

The fix has not been backported to 3.1, since it depends on the fix for issue 7242 (r78527) which was itself never backported to 3.1 after being forward ported to the py3k branch as part of a bulk merge (r83318)

Backporting to 2.7 would also be a manual process (although much easier, since issue 7242 is already fixed in that branch)

Given the obscurity of the error, I'm going to close this as fixed without backporting it. Anyone that wants it fixed in the 2.7 and 3.1 maintenance branches is free to develop and post the requisite patches :)
History
Date User Action Args
2010-12-02 04:36:52ncoghlansetstatus: open -> closed
resolution: fixed
messages: + msg123054

stage: resolved
2010-11-28 03:33:36eric.araujosetassignee: ncoghlan
versions: + Python 3.1, Python 3.2
2010-08-17 21:25:43ncoghlansetmessages: + msg114174
2010-08-17 21:22:47Alex.Roitmansetmessages: + msg114173
2010-08-17 21:18:28gregory.p.smithsetmessages: + msg114171
2010-08-17 21:13:29Alex.Roitmansetmessages: + msg114169
2010-08-17 21:07:05Alex.Roitmansetmessages: + msg114168
2010-08-17 20:56:14ncoghlansetpriority: release blocker -> normal
assignee: barry -> (no value)
messages: + msg114166
2010-08-17 19:59:27gregory.p.smithsetmessages: + msg114153
2010-08-17 18:43:56barrysetmessages: + msg114143
versions: - Python 2.6
2010-08-17 03:01:19ncoghlansetfiles: + fork_on_import.py

messages: + msg114098
2010-08-17 02:58:50ncoghlansetfiles: + issue9573_fork_on_import.diff
keywords: + patch
messages: + msg114097
2010-08-17 02:22:33ncoghlansetmessages: + msg114095
2010-08-17 02:08:42ncoghlansetpriority: normal -> release blocker

nosy: + gregory.p.smith, barry
messages: + msg114094

assignee: barry
2010-08-17 01:04:07eric.araujosetnosy: + eric.araujo
2010-08-13 16:28:00ncoghlansetnosy: + ncoghlan
2010-08-12 14:47:39belopolskysetnosy: - belopolsky
2010-08-12 14:47:25belopolskysetmessages: - msg113654
2010-08-12 05:21:51Alex.Roitmansetmessages: + msg113656
title: imporing a module that executes fork() raises RuntimeError -> importing a module that executes fork() raises RuntimeError
2010-08-12 03:03:45belopolskysetmessages: + msg113654
2010-08-12 02:56:40Alex.Roitmansetmessages: + msg113653
2010-08-12 02:54:57belopolskysetnosy: + belopolsky
messages: + msg113652
2010-08-12 02:48:26Alex.Roitmansetmessages: + msg113651
2010-08-12 02:13:41brett.cannonsetmessages: + msg113649
2010-08-12 02:06:06r.david.murraysetnosy: + r.david.murray, brett.cannon
messages: + msg113648
2010-08-11 23:42:03Alex.Roitmancreate