Skip to content

#110, On Windows ignore ERROR_ACCESS_DENIED for TerminateProcess() if… - #111

Merged
snoyberg merged 1 commit into
haskell:masterfrom
sternmull:issue110
Dec 9, 2018
Merged

snoyberg merged 1 commit into
haskell:masterfrom
sternmull:issue110

Conversation

@sternmull

Copy link
Copy Markdown
Contributor

It seems this can be attributed to strange behavior of the Windows API. libuv has a workaround for this: https://github.com/libuv/libuv/blob/master/src/win/process.c#L1172

It is not perfect (because it does work only for processes that do not exit with 259). But to my current knowledge this is a platform specific limitation we have to accept.

@Mistuke

Mistuke commented Nov 16, 2017

Copy link
Copy Markdown
Contributor

Libuv doesn't swallow the error, it returns UV_ESRCH which is no such process. So these two hammers are not the same. Libuv correctly propagates the fact that the process is still alive. This just pretends it's not, and that's critical for correctness because the process can still return STILL_ALIVE for a number of reasons.

@sternmull

Copy link
Copy Markdown
Contributor Author

The change that i propose only ignores ERROR_ACCESS_DENIED of TerminateProcess() if the process did die (and did not exit with STILL_ACTIVE). In this case we can be sure the process already has terminated. So this change only makes terminateProcess succeed also when the process is already gone (even if TerminateProcess() fails to succeed for that reason).

@Mistuke

Mistuke commented Nov 16, 2017 via email

Copy link
Copy Markdown
Contributor

@sternmull

Copy link
Copy Markdown
Contributor Author

It solves it... except for the situation where the subprocess exits with 259 (STILL_ACTIVE) where the ERROR_ACCESS_DENIED is propagated just like before. So for example my test case from #110 now runs without errors and correctly terminates its subprocesses. See it more as an improvement than a complete solution.

I think we have to accept that there is a chain of obstacles (TerminateProcess() sometimes failing for mysterious undocumented reasons, GetExitCodeProcess() being helpful to work around that... except for one specific exit code) and have to make the best out of it.

@Mistuke

Mistuke commented Nov 16, 2017 via email

Copy link
Copy Markdown
Contributor

@sternmull

Copy link
Copy Markdown
Contributor Author

What makes you think that? The code path that i added only succeeds if the process actually terminated. So you can be sure the process was dead when terminateProcess succeeded.

@Mistuke

Mistuke commented Nov 17, 2017

Copy link
Copy Markdown
Contributor

Oh I see, I misinterpreted your example, the hGetChar is in the caller not the child. fair enough.

@sternmull

Copy link
Copy Markdown
Contributor Author

@snoyberg This is waiting for quite a while now. Could you merge it?

@snoyberg

snoyberg commented Dec 8, 2018

Copy link
Copy Markdown
Collaborator

@Mistuke Are you OK with me merging and including this in the next release?

@Mistuke

Mistuke commented Dec 9, 2018

Copy link
Copy Markdown
Contributor

@snoyberg yes no objections from me. Thanks.

@snoyberg

snoyberg commented Dec 9, 2018

Copy link
Copy Markdown
Collaborator

@sternmull Just one last request then: would you be able to add a short description of the change into the changelog? Thanks!

…ss() if the process did terminate

To my knowledge this behavior is not officially documented. But it seems
multiple people discovered ERROR_ACCESS_DENIED when trying to terminate
a process that did already die on its own. For example libuv has a
workaround for this.
@sternmull

Copy link
Copy Markdown
Contributor Author

@snoyberg sure. Rebased the PR on master and updated changelog.

@snoyberg snoyberg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

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.

3 participants