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

Switch timeout mechanism to subprocess.run #659

Merged
merged 10 commits into from Jan 20, 2023
Merged

Conversation

ConorBobbleHat
Copy link
Collaborator

Okay: hopefully the last timeout-related PR from me for a while :)

This PR:

  • Removes exception_on_timeout & its dependencies (i.e. dill) completely
  • Adds a timeout argument to run_subprocess that passes through to subprocess.run
  • Remove the timeout guard on m2c calls (as it's currently called as a module, not a subprocess)
  • Adds a new compiler, DummyLongRunningCompiler that does nothing but wait for an hour if invoked
  • Adds a new test, test_compiler_timeout, that sets a compilation timeout of three seconds, invokes DummyLongRunningCompiler, and verifies a timeout error was generated
  • Removes the time limits passed to nsjail (to prevent race conditions from having us have to handle two different variants of timeout errors)

Tested on both django's runserver and gunicorn, and verified that that a) both work happily with this timeout mechanism, and b) the impact on latency of compile calls is negligible (within the range of network jitter).

(with many thanks to @simonlindholm for the methodology)

Copy link
Member

@bates64 bates64 left a comment

Choose a reason for hiding this comment

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

These changes all look good! Glad to simplify, and I like the new test. Thanks ^-^

Would you be able to add a test or otherwise verify that COMPILATION_TIMEOUT_SECONDS=0 results in no timeout being applied (i.e. equivalent to passing timeout=None to subprocess.run)?

@ConorBobbleHat
Copy link
Collaborator Author

A timeout of zero should now disable timeouts entirely

@ethteck ethteck merged commit 906aa4a into decompme:main Jan 20, 2023
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.

None yet

3 participants