Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import dotted name as alias breaks with concurrency #74997

Closed
rshk mannequin opened this issue Jun 30, 2017 · 18 comments
Closed

Import dotted name as alias breaks with concurrency #74997

rshk mannequin opened this issue Jun 30, 2017 · 18 comments
Assignees
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@rshk
Copy link
Mannequin

rshk mannequin commented Jun 30, 2017

BPO 30814
Nosy @brettcannon, @ncoghlan, @vstinner, @ned-deily, @ericsnowcurrently, @serhiy-storchaka, @RazerM, @rshk
PRs
  • bpo-30814: Fixed a race condition when import a submodule from a package. #2580
  • [3.6] bpo-30814: Fixed a race condition when import a submodule from a package. (GH-2580). #2598
  • [3.5] bpo-30814: Add a test for concurrent relative import. (GH-2580). #2645
  • bpo-30891: importlib _find_and_load() try/except #2665
  • bpo-30814, bpo-30876: Add new import test files to projects. #2851
  • [3.6] bpo-30814, bpo-30876: Add new import test files to projects. (GH-2851). #2912
  • 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 = 'https://github.com/serhiy-storchaka'
    closed_at = <Date 2017-07-27.10:19:01.622>
    created_at = <Date 2017-06-30.11:09:48.262>
    labels = ['interpreter-core', 'type-bug', '3.7']
    title = 'Import dotted name as alias breaks with concurrency'
    updated_at = <Date 2017-07-27.10:19:01.621>
    user = 'https://github.com/rshk'

    bugs.python.org fields:

    activity = <Date 2017-07-27.10:19:01.621>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2017-07-27.10:19:01.622>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2017-06-30.11:09:48.262>
    creator = 'SamueleSanti'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30814
    keywords = ['3.6regression']
    message_count = 18.0
    messages = ['297380', '297475', '297482', '297679', '297698', '297704', '297803', '297806', '297966', '298050', '298065', '298951', '298958', '299005', '299018', '299019', '299296', '299302']
    nosy_count = 8.0
    nosy_names = ['brett.cannon', 'ncoghlan', 'vstinner', 'ned.deily', 'eric.snow', 'serhiy.storchaka', 'RazerM', 'SamueleSanti']
    pr_nums = ['2580', '2598', '2645', '2665', '2851', '2912']
    priority = 'high'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue30814'
    versions = ['Python 3.6', 'Python 3.7']

    @rshk
    Copy link
    Mannequin Author

    rshk mannequin commented Jun 30, 2017

    I noticed this weird behavior in Python 3.6:

    Apparently import package.module as _alias fails in a threaded environment, sometimes raising an ImportError, seemingly at random.

    Everything seems to be working perfectly fine:

    • On 3.5 and lower
    • When not defining an alias, eg. import package.module
    • When using the from package import module as alias style

    See this repo with code for reproducing the issue:

    https://github.com/rshk/python36-import-alias-concurrency-bug

    I tested this on the system Python 3.6.1 shipped with Archlinux (both inside and outside of a virtualenv), and in the official docker image for 3.6.

    This all started here psycopg/psycopg2#550 as others were experiencing this issue too.

    @serhiy-storchaka serhiy-storchaka added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels Jun 30, 2017
    @serhiy-storchaka
    Copy link
    Member

    See also bpo-23203.

    This was fixed in master in bpo-30024. Maybe it is worth to backport that change to maintained versions?

    @serhiy-storchaka
    Copy link
    Member

    On other side, bpo-23203 fixed just a symptom. The issue is reproduced in master when replace "import foobar.submodule as foo" with "import foobar.submodule; foo = foobar.submodule".

    The regression may be related to bpo-22557. Either the optimization introduced a bug, or it exposed an existing bug by changing timings.

    @serhiy-storchaka serhiy-storchaka added the 3.7 (EOL) end of life label Jul 1, 2017
    @serhiy-storchaka serhiy-storchaka self-assigned this Jul 1, 2017
    @brettcannon
    Copy link
    Member

    My guess is the import of package has completed but package.submodule hasn't (I don't know what state it would be in, but obviously before the module is assigned as an attribute on the package), a context switch occurs, and then the assignment fails due to package.submodule not being set on package yet.

    I wonder if package.submodule is in sys.modules when the AttributeError is triggered? I don't remember when the assignment of a submodule to an attriute on a package occurs, but this suggests it's rather late and that's why this is failing. Maybe the assignment is outside of the per-module lock being released and that is what's causing this?

    Hey, at least importing in a thread doesn't deadlock anymore like it used to. ;)

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 5, 2017

    Documenting explicitly what I believe the expected order of module lock acquisition would be in this case:

    Thread A acquires package.subpackage lock
    Thread B blocks on package.subpackage lock
    Thread A acquires package lock
    Thread A releases package lock
    Thread A releases package.subpackage lock
    Thread B acquires & immediately releases package.subpackage lock
    

    Looking at https://hg.python.org/cpython/rev/64f195790a3a#l4.367 (the commit for bpo-22557), there's a potentially suspect change in the scope of a "_PyImport_AcquireLock/_PyImport_ReleaseLock" pair inside PyImport_ImportModuleLevelObject

    Specifically, I think we may now have a race condition at https://github.com/python/cpython/blob/master/Python/import.c#L1534, where two threads can *both* end up trying to initialize the same module, since we're no longer holding the global import lock around that "mod = PyDict_GetItem(interp->modules, abs_name);" call and the associated state updates where the first thread indicates that it is already initializing that module so the second thread should just wait for it to finish doing so.

    @serhiy-storchaka
    Copy link
    Member

    I have came to the same conclusion.

    PR 2580 adds a double check in _bootstrap._find_and_load() (and also moves the locking into it). It also simplifies the C code by removing the fast path for the case sys.module[name] is None.

    @serhiy-storchaka
    Copy link
    Member

    New changeset b4baace by Serhiy Storchaka in branch 'master':
    bpo-30814: Fixed a race condition when import a submodule from a package. (bpo-2580)
    b4baace

    @serhiy-storchaka
    Copy link
    Member

    New changeset 03b0e83 by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-30814: Fixed a race condition when import a submodule from a package. (GH-2580). (bpo-2598)
    03b0e83

    @brettcannon
    Copy link
    Member

    I just wanted to say thanks to everyone who helped to fix this bug. The locking situation in import is probably the trickiest part of it and has also been tweaked the most as of late and so solving these kinds of issues is tricky.

    @vstinner
    Copy link
    Member

    test_concurrency() of test_import fails randomly on Windows: see bpo-30891.

    @vstinner vstinner reopened this Jul 10, 2017
    @serhiy-storchaka
    Copy link
    Member

    It is worth to backport the test to older versions. Just to be sure that this is a 3.6 regression.

    @ned-deily
    Copy link
    Member

    When running tests from installed location, test_import now fails on master and 3.6 with:

    ======================================================================
    ERROR: test_concurrency (test.test_import.ImportTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/Users/nad/Projects/PyDev/active/dev/3x/root/uxd/lib/python3.7/test/test_import/__init__.py", line 408, in test_concurrency
        raise exc
      File "/Users/nad/Projects/PyDev/active/dev/3x/root/uxd/lib/python3.7/test/test_import/__init__.py", line 393, in run
        import package
    ModuleNotFoundError: No module named 'package'

    @vstinner
    Copy link
    Member

    When running tests from installed location, test_import now fails on master and 3.6 with:

    Which Git revisions are you testing? On master, do you have my latest commit e72b135 ?

    @ned-deily
    Copy link
    Member

    Which Git revisions are you testing? On master, do you have my latest commit e72b135 ?

    Yes, this is with current top of trunk of both branches. Perhaps I wasn't clear about "installed location". The test does not fail if you run it directly from the build directory like with "make test". It fails when you do a "make install" and then run the test using the newly installed Python. Tests need to work both ways.

    @vstinner
    Copy link
    Member

    Serhiy on PR 2851:

    I forget to include directories with new test files in the list of the library directories. This caused that these test files were not installed.

    Oh, too bad that Zach's "x86 Gentoo Installed with X 3.6" buildbot is offline.

    @vstinner
    Copy link
    Member

    I also opened python/devguide#241 : "Document how to add a new file or a new directory". It's not the first time that we make such mistake.

    @serhiy-storchaka
    Copy link
    Member

    New changeset d5ed47d by Serhiy Storchaka in branch 'master':
    bpo-30814, bpo-30876: Add new import test files to projects. (bpo-2851)
    d5ed47d

    @serhiy-storchaka
    Copy link
    Member

    New changeset 95b16a9 by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-30814, bpo-30876: Add new import test files to projects. (GH-2851). (bpo-2912)
    95b16a9

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants