Skip to content

Conversation

Mistuke
Copy link
Contributor

@Mistuke Mistuke commented Dec 18, 2020

Hi,

terminateJob is being called from within terminateProcess which already has a lock on the MVar.
This add a new function terminateJobUnsafe which does not take an MVar and so is not thread-safe but can be called from terminateProcess safely.

Fixes #196

@snoyberg
Copy link
Collaborator

The AppVeyor tests failed on this build.

Side note: I think I'll move CI over to Github Actions instead of the Travis/AppVeyor split, just an FYI.

@Mistuke
Copy link
Contributor Author

Mistuke commented Dec 29, 2020

The AppVeyor tests failed on this build.

That test is somewhat broken which is why it sometimes passes and sometimes fails. See https://sourceforge.net/p/mingw/mailman/mingw-users/thread/CAO2ddnbqqTwABiLSpyDN-ynyQR%3D6fRt5Mt94gszkgQoJF-atOA%40mail.gmail.com/#msg30566147

In general there's no guarantee that the PID inside a Cygwin process is the same as the PID in Windows. Cygwin is trying to emulate a posix environment in user space. It will try to have the PIDs match but this may not always succeed which is why this test is flaky.

In any case, I've updated the test to read the winpid from procfs instead.

@Mistuke Mistuke force-pushed the fix-terminate-process branch 11 times, most recently from 7c9334a to 2856c8d Compare January 3, 2021 14:53
@Mistuke
Copy link
Contributor Author

Mistuke commented Jan 3, 2021

@snoyberg alright, the test is fixed to work properly on Windows now.

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 great. One more request: could you add a changelog entry explaining the change?

@Mistuke Mistuke force-pushed the fix-terminate-process branch from 2856c8d to 904c107 Compare January 3, 2021 15:26
@Mistuke
Copy link
Contributor Author

Mistuke commented Jan 3, 2021

Done.

@snoyberg snoyberg merged commit e4e5c79 into haskell:master Jan 3, 2021
@snoyberg
Copy link
Collaborator

snoyberg commented Jan 3, 2021

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.

use_process_jobs doesn't work with terminateProcess
2 participants