Fix problems with callbacks and killINotify #22

Merged
merged 3 commits into from Dec 3, 2016

Conversation

Projects
None yet
2 participants
@simonmar
Contributor

simonmar commented Nov 30, 2016

  1. We shouldn't run the callback inside mask_, because that prevents it
    from receiving StackOverflow, amongst other things. I think this was
    a n attempt to prevent the delivery of ThreadKilled inside the
    callback, but to do that you would need uninterruptibleMask_

  2. killThread doesn't wait for the threads to die, so use the async
    package and cancel/wait instead.

  3. If the killThread happens during a callback, we don't want to discard
    the exception, because that will leave the thread running.

I've added a new test for the problem of a callback hanging.

simonmar added some commits Nov 30, 2016

Fix problems with callbacks and killINotify
1. We shouldn't run the callback inside mask_, because that prevents it
   from receiving StackOverflow, amongst other things.  I think this was
   a  n attempt to prevent the delivery of ThreadKilled inside the
   callback, but to do that you would need uninterruptibleMask_

2. killThread doesn't wait for the threads to die, so use the async
   package and cancel/wait instead.

3. If the killThread happens during a callback, we don't want to discard
   the exception, because that will leave the thread running.

I've added a new test for the problem of a callback hanging.
@kolmodin

This comment has been minimized.

Show comment
Hide comment
@kolmodin

kolmodin Dec 1, 2016

Owner

Thanks, I'll release a new version with these changes.

On master I had started to port the API to use ByteString instead of strings. It makes some things simpler, like no need to care about file system encoding. I'll wait with those changes and release something with your patches first.

Owner

kolmodin commented Dec 1, 2016

Thanks, I'll release a new version with these changes.

On master I had started to port the API to use ByteString instead of strings. It makes some things simpler, like no need to care about file system encoding. I'll wait with those changes and release something with your patches first.

@simonmar

This comment has been minimized.

Show comment
Hide comment
@simonmar

simonmar Dec 1, 2016

Contributor

Thanks @kolmodin! I worked off the released version because I needed to deploy this locally, but releasing a new version would be great.

Contributor

simonmar commented Dec 1, 2016

Thanks @kolmodin! I worked off the released version because I needed to deploy this locally, but releasing a new version would be great.

@kolmodin kolmodin merged commit 4f770e7 into kolmodin:master Dec 3, 2016

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