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

Bug Fix: Plugins Fail To Load on Python 3.8+ #505

Merged
merged 2 commits into from
Feb 2, 2022

Conversation

jpdakran
Copy link
Member

@jpdakran jpdakran commented Jan 18, 2022

Problem

  • Python 3.8+ multiprocessing library has switched to spawn where before it was fork
  • This is an issue since spawn does not share any memory between the parent and child processes.
  • The plugins/filters are stored as singleton global variable in the Settings class

Solution

  • Pass the settings object to the child processes.

…wn processing which does not share memory between child processes.
@jpdakran jpdakran linked an issue Jan 18, 2022 that may be closed by this pull request
Copy link
Member

@calvinli calvinli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice find, let's hold off on merging this until we get CI set up again though, I'm interested to see whether py38/py39 tests pass on CI

@KevinHock
Copy link
Collaborator

Hi @jpdakran, I also saw #482, as a possible alternative solution.

@domanchi can you review this one? I think you have the most context.

@@ -55,7 +55,8 @@ def scan_files(self, *filenames: str, num_processors: Optional[int] = None) -> N
if not num_processors:
num_processors = mp.cpu_count()

with mp.Pool(processes=num_processors) as p:
# Default to fork multiprocessing. Python 3.8+ defaults to spawn which doesn't share memory.
with mp.get_context('fork').Pool(processes=num_processors) as p:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did find on https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods

that it says fork should not be used on macOS, and that it's not available on Windows

so it may be better to resolve this some other way

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to pass the settings object to the child processes.

@domanchi
Copy link
Contributor

I think it may be better to use the approach with #482 -- essentially, initializing a new settings object per process, rather than sharing memory. It's been a while since I looked at it, but the Settings object was designed for easy serialization/deserialization, so you could pass in a serialized object to the spawned process, and hydrate it when starting it up.

This way, we'd be less dependent on OS level details since this library is used on multiple platforms.

@jpdakran jpdakran merged commit d0df05a into master Feb 2, 2022
@jpdakran jpdakran deleted the jdakran_bugfix_fix_plugin_loading_python_3.8 branch February 11, 2022 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

detect-secrets v1.1.0 fails to load any plugins on py38
4 participants