Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix LocalShellCommand::checkCall() does not kill process on timeout. #741

Merged
merged 1 commit into from
Aug 9, 2022

Conversation

thecoblack
Copy link
Contributor

@thecoblack thecoblack commented Jul 29, 2022

Fixes #677

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 29, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022

Codecov Report

Merging #741 (b58e83b) into development (b58e83b) will not change coverage.
The diff coverage is n/a.

❗ Current head b58e83b differs from pull request most recent head 4d9fa97. Consider uploading reports for the commit 4d9fa97 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##           development     #741   +/-   ##
============================================
  Coverage        88.75%   88.75%           
============================================
  Files              129      129           
  Lines             7855     7855           
============================================
  Hits              6972     6972           
  Misses             883      883           

@ChrisCummins
Copy link
Contributor

Hey @thecoblack, thanks for taking a look at this. Does terminate() have a timeout?

Also, there's a minor indentation issue to fix.

Cheers,
Chris

@thecoblack
Copy link
Contributor Author

Hi @ChrisCummins, terminate() does not provide a timeout. Here is the definition from official docs:

void terminate();
void terminate(std::error_code &) noexcept;

It kills the process through an implementation of SIGKILL on posix and TerminateProcess on windows when is called.

@ChrisCummins
Copy link
Contributor

Thanks for the context, and for the bug fix 🙂 . LGTM, merging!

Cheers,
Chris

@ChrisCummins ChrisCummins merged commit dd66b2c into facebookresearch:development Aug 9, 2022
This was referenced Nov 1, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compiler_gym::util::LocalShellCommand::checkCall() does not kill process on timeout
4 participants