-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix loading tk on windows when current process has >1024 modules. #22445
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
Conversation
Were you able to test this out? One of the key questions from the issue I had is what the API actually does for too-small buffers, as it was unclear from the docs whether it failed, truncated, or something in between. It looks like tests are okay, but the problem with memory corruption is you can never be sure it's not accidentally working. |
Sorry, I could not exercise the pathological case of too-many-modules. OTOH, even though the behavior for too small arrays is not explicitly stated, I think it is implicit in the docs (https://docs.microsoft.com/en-us/windows/win32/api/psapi/nf-psapi-enumprocessmodules):
This strongly suggests that it is OK to call EnumProcessModules with a too-small array. |
I guess the question is more whether it returns an error or not, which would avoid the potential read out of bounds in the original issue, but then also exit early from what you've written. Or maybe it's clever and returns no error with |
@javidcf can you see if this fixes the problem for you? |
I did try that this works to load tk on a standalone process, which means that the first call to EnumProcessModules does not error out. |
Thanks, looks ok as far as I can tell, although I don't have the setup to test my own build of Matplotlib, and unfortunately I probably won't be able to set it up soon as I am currently on leave. If the context helps, this happened when I was trying to use Matplotlib within the Python interpreter in Unreal Engine, with a lot of engine plugins loaded. |
That is a sentence I never expected to read. Are you willing to share any more context? Also in that context do you really need tk windows? |
I don't need Tk windows in particular, I was trying to use Matplotlib to do some visualization and manipulation of data coming from Unreal Engine (e.g. from assets and other things, for tooling purposes). It works, but the default TkAgg backend fails to load, so at the moment I use Qt5Agg. Edit: Sorry, I realised I wasn't clear. In Unreal Engine, Python is only used for editor scripting (a bit like Maya or Blender). I'm not talking about using Matplotlib within an Unreal Engine game, but as a toolkit for developer tools in the Unreal Engine editor. |
…process has >1024 modules.
…445-on-v3.5.x Backport PR #22445 on branch v3.5.x (Fix loading tk on windows when current process has >1024 modules.)
Changed the TKinter module loading function for Windows to support the rare (but possible) case of having more than 1024 modules loaded. This is an adaptation of the same fix that was added to Matplotlib in [PR #22445](matplotlib/matplotlib#22445).
@anntzer @javidcf Thanks for this fix both here and on Pillow! An application I work on (ArcGIS Pro) also recently "broke the 210 module buck" and was causing difficult to assess failures when running from within the application that were not reproducible from standalone Python nor in some cases from within the application. Some digging turned up this issue as the cause, as we were still on releases prior to these fixes being applied. Our base application loads around 400 DLLs, and many Python packages like SciPy have taken the perfectly reasonable route of separating out each component into a separate module, but also means that loading SciPy adds about 100 modules to the running process. Multiplying that across a number of packages can quickly add up to this earlier limit. |
PR Summary
Closes #22378.
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).