-
-
Notifications
You must be signed in to change notification settings - Fork 44
Python (code eval) & dependency version upgrades #246
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
e67219b
to
ebe1afe
Compare
Changes to multiprocessing in Python 3.14 make it more likely that the 5 PID limit is hit even with non-complicated uses of multiprocessing. We have enough compute to allocate more PIDs and safely know this will not affect the operation of other services (we have since migrated our databases and heavier processing applications onto other hosts).
ebe1afe
to
94f61f6
Compare
tests/test_nsjail.py
Outdated
self.assertIn("Resource temporarily unavailable", result.stdout) | ||
self.assertEqual(result.stderr, None) | ||
|
||
self.nsjail.config.cgroup_pids_max = previous_pids_max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in a finally block in case an assertion fails so other tests aren't impacted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amended in ad17dac
object = "A" * 40_000_000 | ||
time.sleep(0.5) | ||
if __name__ == "__main__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forkserver
becoming the default start method means this is required (both forkserver
and spawn
will raise RuntimeError
if the main module cannot be safely imported).
In 3.14, forkserver
became the default for POSIX (Windows and macOS continue to use spawn
), previously fork
was used which did not require this.
https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods
envar: "MKL_NUM_THREADS=5" | ||
envar: "VECLIB_MAXIMUM_THREADS=5" | ||
envar: "NUMEXPR_NUM_THREADS=5" | ||
envar: "OMP_NUM_THREADS=15" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the increase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind I read the commit message
This test case was being caught and nsjail was killing it, but not because of PID exhaustion but memory exhaustion. To ensure PID exhaustion is guarded against the PID count is now reduced to a level where nsjail will kill the process earlier before the memory limit.
94f61f6
to
ad17dac
Compare
See python-discord/snekbox#246 for relevant snekbox changes
Note
This does not change the version of Python used to run the snekbox API and
wrap around nsjail, just the versions that are available within the execution
environment.
This PR: