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

Using async to properly handle exceptions when multiple threads are run #76

Closed

Conversation

qrilka
Copy link
Contributor

@qrilka qrilka commented Mar 13, 2017

No description provided.

@qrilka
Copy link
Contributor Author

qrilka commented May 19, 2017

@tmcgilchrist @markhibberd is something required for this to be merged?
We were bitten by this bug in production

riak.cabal Outdated
@@ -100,6 +100,7 @@ library

build-depends:
aeson >= 0.8 && < 1.2,
async,
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind putting some reasonable bounds on this dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

amended the commit with >= 2.0.0.0

@tmcgilchrist
Copy link
Member

Do you have some investigation notes from the bug you encountered?

If this change fixed it I will merge but I'm still not convinced the code is doing the right thing under exceptions. We've been bitten badly with async exceptions lately and I'm suspicious of everything :-)

@qrilka
Copy link
Contributor Author

qrilka commented May 22, 2017

@tmcgilchrist that was 3 months ago and unfortunately I haven't done detailed notes regarding this bug.
But as far as I remember the main problem was that our data processing code reading Riak sometimes was just stuck without giving any logs or something. This patch fixed the situation.
The problem in our code was cause by getMany function with was calling pipe under the hood.
And in unpatched pipe any exception in replicateM_ numReqs $ writeChan ch =<< receive conn will just kill sending thread leaving the main one waiting forever for results in channel ch.
The patch adds more safety as asyncBoth re-throws child thread exception if it happens.

@tmcgilchrist
Copy link
Member

Thanks for the details, I'll push out a new version once the builds are all good.

@tmcgilchrist
Copy link
Member

Merged here #77

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