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

Make `cancel` uninterruptible... #44

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@bitonic
Contributor

bitonic commented Jul 11, 2016

...create interruptibleCancel, update docs.

CC @snoyberg @kantp

Make `cancel` uninterruptible...
...create `interruptibleCancel`, update docs.
@bitonic

This comment has been minimized.

Contributor

bitonic commented Jul 11, 2016

Note that this fixes #43

@simonmar

This comment has been minimized.

Owner

simonmar commented Jul 11, 2016

@snoyberg you said here #41 (comment) that you wouldn't mind the waitCatch in cancel being interruptible, but what we ended up with (I only just realised) is that it is uninterruptible in the context of withAsync, and this ticket argues for it always to be uninterruptible.

Do we really want either of those?

Modify concurrently' to correctly match the semantics of cancel
The waiting for the thread to die is not interruptible too.
@bitonic

This comment has been minimized.

Contributor

bitonic commented Jul 12, 2016

@simonmar me and @snoyberg had various discussions after that comment and he agreed that uninterruptible cancel by default is the way to go. I think he'll also be available later today so he can reply himself :)

kantp pushed a commit to fpco/libraries that referenced this pull request Jul 12, 2016

@simonmar

This comment has been minimized.

Owner

simonmar commented Jul 12, 2016

could you make fde3c6d a separate pull request please?

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jul 12, 2016

I was mistaken in that comment, as @bitonic convinced me. We definitely need to deliver the exception to the child thread, so making the cancel uninterruptible makes sense. There's still an argument for making the waitCatch interruptible.

@simonmar

This comment has been minimized.

Owner

simonmar commented Jul 12, 2016

@snoyberg it was the waitCatch I was talking about (and what your earlier comment was about). Since uninterruptibleCancel now includes the waitCatch, the waitCatch in withAsync is now uninterruptible. Is that what we want?

@bitonic bitonic closed this Jul 12, 2016

@bitonic bitonic referenced this pull request Jul 12, 2016

Merged

Cancel adjustments #45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment