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

Add support for watching processes with IOCP #73

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

kcbanner
Copy link
Contributor

@kcbanner kcbanner commented Oct 29, 2023

These changes add support for receiving events from JobObjects (https://learn.microsoft.com/en-us/windows/win32/procthread/job-objects) in the IOCP backend.

This feature is then used to implement an IOCP process watcher.

JobObjects are unique with the way they interact with the OVERLAPPED_ENTRY result structure - they completely repurpose all the fields to mean different things. This means I fill in the completion result directly before perform is called. Let me know if this is an issue, I could rework perform to accept the entry itself.

Remaining TODOs:

  • Figure out how to address the race condition that exists if the process exits before wait is called - there will be no completion events on the JobObject in this case

watcher: Add support for watching processes using IOCP
Previously, there was a race condition in the IOCP process watcher where
if the process exited before wait() was called, it would hang indefinitely.

I addressed this by adding a new result type for when a job is associated
with the completion port, and calling the callback with this result so that
the user code has a chance to check any invariants.

Changes:
- Call the completion callback when the job is associated with the IOCP
- Add a test that tests the case of the process exiting before the wait() call
- Rework job object handling to not require @unionInit. The user code
  can now interpret the message data however they want
@kcbanner kcbanner marked this pull request as ready for review October 31, 2023 05:05
@kcbanner
Copy link
Contributor Author

kcbanner commented Oct 31, 2023

Just pushed a change fixing the race condition mentioned in the PR description, see the commit message for details. Moving this out of draft status now.

@mitchellh
Copy link
Owner

Looks great. We're getting regular errors (retried a couple times) in CI. Any thoughts on those?

@kcbanner
Copy link
Contributor Author

I don't see those locally, but it might be because the process tree in the CI is running under a job object itself - probably to control resource limits. I'll see if I can reproduce the same conditions locally and rework the logic to test if the process is already in a job object, and use that one instead.

@mitchellh
Copy link
Owner

I don't see those locally, but it might be because the process tree in the CI is running under a job object itself - probably to control resource limits. I'll see if I can reproduce the same conditions locally and rework the logic to test if the process is already in a job object, and use that one instead.

Thanks. Also happy to reconfigure CI if you have a way to do that. Either way, I'd love for the CI to pass. 😄

@kcbanner
Copy link
Contributor Author

I'm not sure until this CI run happens, but I think the issue was trying to set the JOB_OBJECT_LIMIT_SILENT_BREAKAWAY_OK on the job - this would have let child processes of the child process spawned during the test not be part of their parent job - which depending on the settings of the parent job, may not be allowed.

I did this initially because it would avoid having to handle (and ignore) events from any subprocesses. I reworked the logic to not set this, and just check the PIDs instead.

@mitchellh mitchellh merged commit 1b46c2d into mitchellh:main Oct 31, 2023
10 checks passed
@mitchellh
Copy link
Owner

Awesome. Thank you!

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.

2 participants