Skip to content

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

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 10, 2022

PR Summary

Closes #22378.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@QuLogic
Copy link
Member

QuLogic commented Feb 10, 2022

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.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 10, 2022

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):

To determine if the lphModule array is too small to hold all module handles for the process, compare the value returned in lpcbNeeded with the value specified in cb. If lpcbNeeded is greater than cb, increase the size of the array and call EnumProcessModules again.

This strongly suggests that it is OK to call EnumProcessModules with a too-small array.

@QuLogic
Copy link
Member

QuLogic commented Feb 11, 2022

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 NULL buffer.

@tacaswell tacaswell added this to the v3.5.2 milestone Feb 11, 2022
@QuLogic
Copy link
Member

QuLogic commented Feb 11, 2022

@javidcf can you see if this fixes the problem for you?

@anntzer
Copy link
Contributor Author

anntzer commented Feb 11, 2022

Or maybe it's clever and returns no error with NULL buffer.

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.

@javidcf
Copy link

javidcf commented Feb 11, 2022

@javidcf can you see if this fixes the problem for you?

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.

@tacaswell
Copy link
Member

...when I was trying to use Matplotlib within the Python interpreter in Unreal Engine....

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?

@javidcf
Copy link

javidcf commented Feb 11, 2022

...when I was trying to use Matplotlib within the Python interpreter in Unreal Engine....

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.

@QuLogic QuLogic merged commit 8f1758b into matplotlib:main Mar 3, 2022
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Mar 3, 2022
QuLogic added a commit that referenced this pull request Mar 3, 2022
…445-on-v3.5.x

Backport PR #22445 on branch v3.5.x (Fix loading tk on windows when current process has >1024 modules.)
@anntzer anntzer deleted the wtk branch March 3, 2022 08:41
javidcf added a commit to javidcf/Pillow that referenced this pull request Nov 25, 2022
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).
@scdub
Copy link

scdub commented Mar 29, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: TkAgg fails to find Tcl/Tk libraries in Windows for processes with a large number of modules loaded
5 participants