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

Python 3.5.0rc3 on Windows can not load more than 127 C extension modules #69215

Closed
cgohlke mannequin opened this issue Sep 8, 2015 · 19 comments
Closed

Python 3.5.0rc3 on Windows can not load more than 127 C extension modules #69215

cgohlke mannequin opened this issue Sep 8, 2015 · 19 comments
Assignees
Labels

Comments

@cgohlke
Copy link
Mannequin

cgohlke mannequin commented Sep 8, 2015

BPO 25027
Nosy @pfmoore, @larryhastings, @tjguk, @skrah, @zware, @eryksun, @zooba
Files
  • test_dll_load_failed.py
  • 25027_1.patch
  • 25027_2.patch
  • 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/zooba'
    closed_at = <Date 2015-09-12.15:31:38.716>
    created_at = <Date 2015-09-08.08:05:36.919>
    labels = ['extension-modules', 'OS-windows', 'release-blocker']
    title = 'Python 3.5.0rc3 on Windows can not load more than 127 C extension modules'
    updated_at = <Date 2015-09-12.15:31:38.715>
    user = 'https://bugs.python.org/cgohlke'

    bugs.python.org fields:

    activity = <Date 2015-09-12.15:31:38.715>
    actor = 'larry'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2015-09-12.15:31:38.716>
    closer = 'larry'
    components = ['Extension Modules', 'Windows']
    creation = <Date 2015-09-08.08:05:36.919>
    creator = 'cgohlke'
    dependencies = []
    files = ['40402', '40410', '40418']
    hgrepos = []
    issue_num = 25027
    keywords = ['patch']
    message_count = 19.0
    messages = ['250169', '250184', '250189', '250195', '250216', '250222', '250224', '250226', '250227', '250240', '250256', '250262', '250272', '250273', '250279', '250287', '250296', '250313', '250522']
    nosy_count = 9.0
    nosy_names = ['paul.moore', 'larry', 'tim.golden', 'cgohlke', 'skrah', 'python-dev', 'zach.ware', 'eryksun', 'steve.dower']
    pr_nums = []
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue25027'
    versions = ['Python 3.5']

    @cgohlke
    Copy link
    Mannequin Author

    cgohlke mannequin commented Sep 8, 2015

    This issue was first mentioned at <http://bugs.python.org/issue24872#msg249591\>.

    The attached script (test_dll_load_failed.py) builds up to 256 C extension modules and imports them. On Python 3.4.3 the script passes while on Python 3.5.0rc3 the script fails with:

    Traceback (most recent call last):
    File "test_dll_load_failed.py", line 42, in <module>
    import_module(name)
    File "X:\Python35\lib\importlib\__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
    File "<frozen importlib._bootstrap>", line 986, in _gcd_import
    File "<frozen importlib._bootstrap>", line 969, in _find_and_load
    File "<frozen importlib._bootstrap>", line 958, in _find_and_load_unlocked
    File "<frozen importlib._bootstrap>", line 666, in _load_unlocked
    File "<frozen importlib._bootstrap>", line 577, in module_from_spec
    File "<frozen importlib._bootstrap_external>", line 903, in create_module
    File "<frozen importlib._bootstrap>", line 222, in _call_with_frames_removed
    ImportError: DLL load failed: A dynamic link library (DLL) initialization routine failed.
    

    Tested on Windows 7 and 10, 64 bit, with Python 3.5.0rc3, 32 and 64 bit.

    Due to this issue the "scientific stack" is practically unusable. For example, Pandas unit tests error or abort. In a Jupyter notebook, the following simple imports fail (using current builds from <http://www.lfd.uci.edu/~gohlke/pythonlibs/\>):

    In [1]:
    
    import matplotlib.pyplot
    import pandas
    import statsmodels.api
    ---------------------------------------------------------------------------
    ImportError                               Traceback (most recent call last)
    <ipython-input-1-69fcce4de550> in <module>()
          1 import matplotlib.pyplot
          2 import pandas
    ----> 3 import statsmodels.api
    
    <snip>
    
    X:\Python35\lib\site-packages\scipy\signal\__init__.py in <module>()
        276 # The spline module (a C extension) provides:
        277 #     cspline2d, qspline2d, sepfir2d, symiirord1, symiirord2
    --> 278 from .spline import *
        279 
        280 from .bsplines import *
    
    ImportError: DLL load failed: A dynamic link library (DLL) initialization routine failed.
    

    The cause of this issue is that as of Python 3.5.0rc1 C extension modules are linked statically to the multi-threaded runtime library (/MT) instead of the multi-threaded DLL runtime library (/MD). A process can not load more than 127 statically-linked CRT DLLs using LoadLibrary due to a limit of fiber-local storage (FLS) as mentioned in the following links:

    <http://stackoverflow.com/questions/1437422\>
    <https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/3546c3c4-1b36-4552-85c5-1b3ba860ee84\>

    To put the 127 limit in perspective: the pywin32 package contains 51 C extension modules, pygame 36, scipy 65, and scikit-image 41.

    In addition to C extension modules, the 127 limit also applies to statically-linked CRT DLLs that are dynamically loaded via Ctypes or LoadLibrary.

    @cgohlke cgohlke mannequin added extension-modules C modules in the Modules dir OS-windows labels Sep 8, 2015
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 8, 2015

    If the scientific stack is unusable, I think this should be a release
    blocker.

    @skrah skrah mannequin added the release-blocker label Sep 8, 2015
    @larryhastings
    Copy link
    Contributor

    This is your wheelhouse, Steve.

    @zooba
    Copy link
    Member

    zooba commented Sep 8, 2015

    Let me experiment today with a few of the proposals I posted in the other thread and get back to you.

    I suspect someone will need to ship vcruntime.dll, and I'd rather it was the extension.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 8, 2015

    Is Python-core built with /MD? I cannot see the flags in the buildbot
    logs.

    @zooba
    Copy link
    Member

    zooba commented Sep 8, 2015

    Kind-of... We use the same flags I described in my blog1 so that we don't have any version-specific dependencies.

    You should (might) see /MT in the build logs, but then we replace most of the static CRT with the dynamic (but versionless) one. The versioned parts (including the FlsAlloc call - module initialization is compiler version specific) are statically linked.

    I'm going to try and update distutils to build with /MD again (sorry Christoph!) and include vcruntime###.dll in the output. That way, people who bdist_wheel will include all of their own dependencies and don't have to worry about whether users are on Python 3.5.0 or 3.9.9.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 8, 2015

    It seems to be /MTd. Sorry for the noise (and yay! for horizontal scrolling :).

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Sep 8, 2015

    The reason I asked: We had issues where extension modules linked
    against a different CRT had isolated locale settings from the
    interpreter, i.e., changes made by the module would not be seen
    by the interpreter.

    I don't know if this is still an issue with the new runtimes.

    @zooba
    Copy link
    Member

    zooba commented Sep 8, 2015

    It shouldn't be - locale state is in the shared part of the CRT. That is one of the reasons I was keen to move to this model (everyone seems to think that FILE* is the only problem with mixing CRT versions :) )

    @zooba
    Copy link
    Member

    zooba commented Sep 8, 2015

    Attached a fix for distutils that will include the required vcruntime DLL with the extension.

    This is purely Python source code and only needs to be patched on the build machine.

    I have tested with a numpy build from source (setup.py bdist_wheel) and it works correctly on a clean Win7 machine with only a Python 3.5.0rc3 install.

    When multiple vcruntime###.dll files are available, the first one that is loaded wins. This means that if you can guarantee a particular import occurs first, you can omit the DLL elsewhere in your package. There is no way to determine this automatically,

    Because of the way I wrote the patch, if you build with DISTUTILS_USE_SDK set, you will get the old static linking unless you also define PY_VCRUNTIME_REDIST to the path of the file to include. If the redist file cannot be found (you probably don't have the compiler either, but assuming you do), you will get the old static linking. I think this is the right balance of "works-by-default" and "I know what I'm doing let me control it" (though now I put them next to each other, I could rename the variable to DISTUTILS_VCRUNTIME_REDIST).

    Will work up a test for it, but wanted to get feedback on the approach first.

    @cgohlke
    Copy link
    Mannequin Author

    cgohlke mannequin commented Sep 8, 2015

    I understand that distributing dependent DLLs next to extension modules is considered the best approach <https://mail.python.org/pipermail/distutils-sig/2014-October/024990.html\> (which nevertheless fails in common cases), however vcruntime140.dll is a special case since it will be shared by almost all extension modules and can be considered a system library.

    My Python 3.4 installation contains 913 .pyd files in 277 directories under Lib\site-packages. With the proposed change there will be ~277 redundant vcruntime140.dll files under Python 3.5. Size is not an issue since vcruntime140.dll is small (~87 KB for 64 bit).

    Many extension modules are installed directly into Lib\site-packages (no package directory). Uninstalling any one of those extension modules using pip or "wininstaller" will delete vcruntime140.dll from Lib\site-packages, potentially breaking the other extension modules in Lib\site-packages.

    IANAL, but under GPL "you may not distribute these [runtime] libraries in compiled DLL form with the program" <http://www.gnu.org/licenses/gpl-faq.html#WindowsRuntimeAndGPL\>.

    @zooba
    Copy link
    Member

    zooba commented Sep 8, 2015

    vcruntime140.dll *is* a system library when installed properly, and if someone installs VCRedist then all the bundled ones should be ignored. Over time, I expect to see extensions appear that depend on vcruntime150.dll rather than 140.dll, so it won't always be shared by all extensions.

    However, if someone has vcruntime140.dll installed and an extension requires vcruntime150.dll, they will get errors unless that extension includes the correct version. We can't ship currently-nonexistent versions with Python 3.5, and if extensions have to depend on what's installed then Python 3.5 extensions will forever be tied to MSVC 14.0.

    GPL code should either statically link or recommend their users install VCRedist separately. I'm not going to compromise compiler version independence because of one license.

    The uninstall issue is something I hadn't considered. Maybe we can special-case pip uninstalling it from the site-packages folder? *Paul* - any thoughts?

    @eryksun
    Copy link
    Contributor

    eryksun commented Sep 9, 2015

    I think 3.5 should be distributed with vcruntime140.dll. It seems a waste for python.exe, python35.dll, and all of the extension modules and dependent DLLs to each take an FLS slot:

        >>> import ctypes # +1 for _ctypes
        >>> kernel32 = ctypes.WinDLL('kernel32')
        >>> kernel32.FlsGetValue.restype = ctypes.c_void_p
        >>> [x for x in range(128) if kernel32.FlsGetValue(x)]
        [1, 2, 4, 5, 6, 8]
    
        >>> import pyexpat, select, unicodedata, winsound
        >>> import _bz2, _decimal, _elementtree, _hashlib
        >>> import _lzma, _msi, _multiprocessing, _overlapped 
        >>> import _socket, _sqlite3, _ssl, _tkinter
        >>> [x for x in range(128) if kernel32.FlsGetValue(x)]
        [1, 2, 4, 5, 6, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27]

    @zooba
    Copy link
    Member

    zooba commented Sep 9, 2015

    Okay, here's a proposal:

    We bundle vcruntime140.dll with Python's normal install, so it's always there and extensions that use it do not need to ship anything.

    When distutils._msvccompiler is used to build an extension with a *different* version of MSVC, it will copy the dependency or statically link (as in my attached patch).

    This does not prevent us from changing the compiler used for 3.5, as long as we continue to ship both vcruntime140.dll and the newer version, and extensions build with newer compilers will include the dependency or pick up the bundled one if they are on a version that includes it.

    (Extensions that use C++ and depend on msvcp###.dll will need to ship that themselves, obviously.)

    I'll post a new patch shortly, but it's only a very small change from this one for distutils, and the rest is in the installer.

    @zooba
    Copy link
    Member

    zooba commented Sep 9, 2015

    New patch. Mostly build and installer changes, but the distutils/_msvccompiler.py is also part of it.

    I've run a full build and done basic testing with a full test run going now, but I don't have a clean machine handy to try it without the full CRT install, so that'll have to wait until tomorrow.

    This *basically* reverts the build back to /MD for everything. The one exception is that distutils now knows which DLLs are shipped with Python and if a vcruntime is needed that isn't included, it will be put into the dist (or statically linked). So wheels created with 3.5.6 and MSVC 15.0 will still run against 3.5.0, even if the user has not installed the latest VCRedist.

    @zooba
    Copy link
    Member

    zooba commented Sep 9, 2015

    FYI: we're making a new release (right now!) with the patch applied, that should go out tomorrow.

    If anyone spots anything important in the patch, I still really want to hear about it, but hopefully having something installable means we'll get at least a few days of testing before locking it in.

    All my apologies for waiting until the last minute before giving up on the crusade to avoid including the versioned files in Python. The timing is unfortunate, but I'm sure we're going to have the best compatibility story possible right now. So thanks for indulging me, and I hope I haven't put too many people through too much anguish.

    @pfmoore
    Copy link
    Member

    pfmoore commented Sep 9, 2015

    Maybe we can special-case pip uninstalling it from the site-packages folder? *Paul* - any thoughts?

    Sorry, I've been following this thread but it's been moving pretty fast, so I'm probably replying too late to be helpful now :-(

    One alternative thought I had was to bundle vcruntime140.dll in a separate wheel, which other wheels can depend on. Then we get pip's usual dependency resolution to handle ensuring that the runtime is present.

    It's possible to special-case vcruntime, what I'd do is modify distutils to omit vcruntime140.dll from the RECORD file - then nothing (pip, distlib, other install tools) views the vcruntime file as being "owned" by a particular package. I'm not overly keen on that solution, though, as it's clearly a hack and would be a maintainability issue going forward. But it does keep the code changes isolated to the core, rather than needing pip/setuptools changes.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 9, 2015

    New changeset 8374472c6a6e by Steve Dower in branch '3.5':
    Issue bpo-25027: Reverts partial-static build options and adds vcruntime140.dll to Windows installation.
    https://hg.python.org/cpython/rev/8374472c6a6e

    @larryhastings
    Copy link
    Contributor

    Pull request accepted. Please forward-merge, thanks!

    @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
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants