Repository navigation
PyErr_Warn may cause import deadlock #37950
Description
| BPO | 683658 |
|---|---|
| Nosy | @gvanrossum, @mhammond, @tiran |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2008-01-19.20:29:54.502>
created_at = <Date 2003-02-10.00:26:08.000>
labels = ['interpreter-core']
title = 'PyErr_Warn may cause import deadlock'
updated_at = <Date 2008-01-19.20:29:54.501>
user = 'https://github.com/mhammond'bugs.python.org fields:
activity = <Date 2008-01-19.20:29:54.501>
actor = 'georg.brandl'
assignee = 'none'
closed = True
closed_date = <Date 2008-01-19.20:29:54.502>
closer = 'georg.brandl'
components = ['Interpreter Core']
creation = <Date 2003-02-10.00:26:08.000>
creator = 'mhammond'
dependencies = []
files = ['787', '788', '789']
hgrepos = []
issue_num = 683658
keywords = ['patch']
message_count = 21.0
messages = ['14540', '14541', '14542', '14543', '14544', '14545', '14546', '14547', '14548', '14549', '14550', '14551', '14552', '14553', '14554', '14555', '14556', '14557', '59186', '59190', '59191']
nosy_count = 6.0
nosy_names = ['gvanrossum', 'jhylton', 'mhammond', 'nnorwitz', 'jvr', 'christian.heimes']
pr_nums = []
priority = 'low'
resolution = None
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue683658'
versions = ['Python 2.5']
Activity
mhammond commentedon Feb 10, 2003
PyErr_Warn() does an implicit import. Thus, if
PyErr_Warn() is called on a thread while the main
thread holds the import lock, and the main thread then
subsequently waits for the child thread, Python will
deadlock. The builtin 'apply' now calls PyErr_Warn(),
so simply calling 'apply' on another thread may cause
the deadlock.
Attaching a sample case. Executing 'python -c "import
hang_apply"' will cause Python to deadlock. Commenting
out the call to "apply" in that file will prevent the
deadlock (even though the apply() in question is
effectively a no-op)
The example is a little contrived, but is extracted
from real code of mine that saw this hang. The code
path is:
calls apply()
attempts to acquire the import lock.
complete, but still holds import lock.
Note that removing the call to PyErr_Warn() in
builtin_apply also solves this particular hang -
however, it seems like this is a potential time bomb.
A potential solution would be to prevent PyErr_Warn
from doing an import - this would mean importing
'warnings' at startup, and keeping a static reference
in errors.c. Other thoughts welcome.
nnorwitz commentedon Feb 11, 2003
Logged In: YES
user_id=33168
I can't think of any other/better solution. Any idea if
there are there other ticking bombs like this?
mhammond commentedon Feb 14, 2003
Logged In: YES
user_id=14198
This simple strategy doesn't work - avoiding the import of
'warnings' works fine, until 'warnings' imports 'linecache'!
I'll look at how we can simply throw the warning away if a
deadlock would occur.
mhammond commentedon Feb 15, 2003
Logged In: YES
user_id=14198
The best I can come up with here is exposing the import lock
to the C API, with a "wait" flag.
This will allow a C function to reliably determine if an
"import" work block, or acquire the lock if not. It can
then complete its import before releasing the lock. If the
import would block, then it can take whatever action is
necessary - in the case of PyErr_Warn, it dumps the warning
to stdout.
Attaching a patch. It exposes (to the core, but not in
headers) two functions:
extern int PyImport_LockImport(int wait);
extern void PyImport_UnlockImport(void);
PyErr_Warn then uses these.
If we do go this route, it makes sense to make these
functions truly public (ie, add them to the headers), and
cleanup import.c appropriately. I didn't do this in the
interests of keeping the patch small so it can be easily
digested.
Comments? Other ideas?
gvanrossum commentedon Feb 15, 2003
Logged In: YES
user_id=6380
No time to review everything here, but maybe PyErr_Warn
should not do an import? The import could be done at startup
time (when site.py is also imported) and saved in the
interpreter state.
gvanrossum commentedon Feb 15, 2003
Logged In: YES
user_id=6380
Oh, and the import of linecache by warnings can be put at
the top of warnings, if that's an issue.
mhammond commentedon Feb 15, 2003
Logged In: YES
user_id=14198
Yes, the linecache import was the problem. I abandonded
that approach mainly to avoid yet another import at startup.
Should 'warnings' get new features, all required imports in
the future will also need to be imported at process startup.
It also struck me as a time bomb.
I will make an alternative patch that moves the imports.
mhammond commentedon Feb 19, 2003
Logged In: YES
user_id=14198
Checking in a patch that avoids the import, thereby avoiding
the hang. As suggested, the "linecache" import in warnings
had to be moved.
Even though I checked it in, I attach the patch here, and
have assigned it to Guido for review. If happy, just mark
it as closed.
/cvsroot/python/python/dist/src/Python/errors.c,v <-- errors.c
new revision: 2.76; previous revision: 2.75
/cvsroot/python/python/dist/src/Python/pythonrun.c,v <--
pythonrun.c
new revision: 2.178; previous revision: 2.177
/cvsroot/python/python/dist/src/Lib/warnings.py,v <--
warnings.py
new revision: 1.19; previous revision: 1.18
gvanrossum commentedon Feb 19, 2003
Logged In: YES
user_id=6380
I'm not happy with this solution, since it breaks for
multiple interpreters. So keeping this open. Will think
about what to do instead after 2.3a2 is released.
jvr commentedon Feb 25, 2003
Logged In: YES
user_id=92689
Here's an issue with the patch I just ran into: it seems this patch causes the first Python module to be imported to be warnings.py instead of site.py. This means site.py doesn't have full control of the import mechanism anymore, as all modules loaded by warnings.py will already be loaded. Can't the PyModule_WarningsModule global be set _after_ site.py has run?
Use case: I'm building standalone applications in which the import bootstrapping happens in a custom site.py, assuming it will be the first Python module that will be imported. It sets up sys.path appropriately. Now it turns out that when my site.py gets run (eg.) os.py is already loaded from my system-wide installation, instead of from the zip file embedded in the application. This means I can't reliably test whether my built app really works when there's no Python installation available.
gvanrossum commentedon Feb 25, 2003
Logged In: YES
user_id=6380
Good point. But loading site.py first means that warnings
issued as a result of importing site.py are not handled
properly (they will be dumped to stderr, which is better
than nothing). Well, I have to revisit this anyway; I'll
make sure to take this issue into account as well.
jvr commentedon Feb 25, 2003
Logged In: YES
user_id=92689
Additional info regarding the current situation (with the patch in place):
if warnings.py can't be imported before site.py is run, _all_ warnings issued from C will be printed to stderr, even if warnings.py _can_ be imported later.
I think it's fair to assume that any warning issued during the import of site.py signifies that something is pretty broken, so having them dumped to stderr isn't all that bad.
If you and Mark agree, I'll move the import of warnings.py to after initsite().
gvanrossum commentedon Feb 25, 2003
Logged In: YES
user_id=6380
OK, as a stopgap measure that sounds like a good idea.
(I wonder if -S should also suppress loading warnings? Or is
that too drastic?)
jvr commentedon Feb 25, 2003
Logged In: YES
user_id=92689
Only if PyErr_Warn() would still also attempt to do the import if PyModule_WarningsModule is NULL. But that means that deadlocks can still occur when -S is used. But I _do_ like it that no file system imports are done by Python itself with -S. And I actually don't like it at all that warnings.py always gets imported, even if it's never needed.
gvanrossum commentedon Feb 25, 2003
Logged In: YES
user_id=6380
OK, implement that.
jvr commentedon Feb 25, 2003
Logged In: YES
user_id=92689
Well, since you said the issue needs to be revisited anyway, I'll just leave it at moving the warnings.py import after initsite().
What I don't like is that warnings.py is imported preemptively to begin with, not just with -S. Whether that's resolvable I don't know, but if it is, it would be better than not importing warnings.py with -S.
jhylton commentedon Jun 29, 2003
Logged In: YES
user_id=31392
Did this get resolved for 2.3 or delayed to 2.4?
jvr commentedon Jun 29, 2003
Logged In: YES
user_id=92689
This is from the CVS log of pythonrun.c:
revision 2.179
date: 2003/02/25 20:25:12; author: jvr; state: Exp; lines: +2 -2
Addendum to bpo-683658:
import warnings.py _after_ site.py has run. This ensures that
site.py is again the first .py to be imported, giving it back full
control over sys.path.
I'm quite confident this is good enough for 2.3, but the issue
should be revisited at some point, so I'm not sure we should close
this bug. I'll lower the priority.
tiran commentedon Jan 4, 2008
Is this still an issue for 2.5? If Python 2.5 may still dead lock then
somebody should back port my PyImport_ImportModuleNoBlock() function.
gvanrossum commentedon Jan 4, 2008
No, backporting that to 2.5 isn't an option; it introduces a new feature
and breaks binary backwards compatibility by removing the
PyImport_ImportModuleEx entry point from the executable (turning it into
a macro).
gvanrossum commentedon Jan 4, 2008
Let's close this unless there's a response from one of the original
stakeholders within a week (i.e. by Jan 11 2008).