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

Windows: Check if all processes really exited and released resources #159

Merged

Conversation

Mistuke
Copy link
Contributor

@Mistuke Mistuke commented Nov 5, 2019

The PR makes sure we're only listening to completion events for the key that we're interested in. This isn't much of an issue now but the new I/O manager for Windows will exclusively use I/O completion ports. Which means this method may mistakenly handle events it shouldn't have.

Secondly it fixes a rare but annoying condition. When you receive a notification on the port that a process has terminated this does not mean that the resources of the process has been flushed.

Previously we were immediately unregister the process once we get the notification. However things like hsc2hs which rely on the child process having written a file then may get into a race condition. Control may return to the caller before the file has actually been written.

Windows deals with this by returning an exit code STILL_ACTIVE which is to be interpreted as "process has exited but has not yet been cleaned up". If we get such an event we then block and wait for the OS to signal the handle and re-set the exit code.

This fixes https://gitlab.haskell.org/ghc/ghc/issues/17108 and https://gitlab.haskell.org/ghc/ghc/issues/17249

Copy link
Collaborator

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

Looks good. Can you add a ChangeLog entry?

@Mistuke Mistuke force-pushed the wip/wait-for-actual-process-termination branch from be0d0d8 to 8cb23fd Compare November 5, 2019 08:00
@Mistuke
Copy link
Contributor Author

Mistuke commented Nov 5, 2019

Done :)

Copy link
Collaborator

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@snoyberg snoyberg merged commit 6f957bc into haskell:master Nov 5, 2019
@Mistuke Mistuke deleted the wip/wait-for-actual-process-termination branch November 5, 2019 08:19
bgamari added a commit to ghc/ghc that referenced this pull request Nov 5, 2019
This should fix the #17108 and #17249 with the fix from
haskell/process#159.
bgamari added a commit to ghc/ghc that referenced this pull request Nov 5, 2019
This should fix the #17108 and #17249 with the fix from
haskell/process#159.
bgamari added a commit to ghc/ghc that referenced this pull request Nov 7, 2019
This should fix the #17108 and #17249 with the fix from
haskell/process#159.
bgamari added a commit to ghc/ghc that referenced this pull request Nov 7, 2019
This should fix the #17108 and #17249 with the fix from
haskell/process#159.
bgamari added a commit to ghc/ghc that referenced this pull request Nov 8, 2019
This should fix the #17108 and #17249 with the fix from
haskell/process#159.
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