-
Notifications
You must be signed in to change notification settings - Fork 192
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
Check for connection status in finalizer #226
Conversation
@cdepillabout Could you please check that it fixes the issue for you. |
@@ -93,7 +93,13 @@ makeConnection r w c = do | |||
-- already closed connection. | |||
closedVar <- newIORef False | |||
|
|||
_ <- mkWeakIORef istack c | |||
let close = do | |||
closed <- readIORef closedVar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not certain it's necessary, but to avoid a potential race condition, what about using atomicModifyIORef
to read the closed state and immediately write back True
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize that's a change to how the code works now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Kill read thread on exit from a server. Otherwise it reads from already closed socket, making harder to debugging similar issues in the library itself. Ignore IO errors in write and read thread. They pollute the output without good reason.
6de70b9
to
2046a52
Compare
I'm not going to be able to test this until Tuesday. But the fix looks pretty close to what I described in #225, which did work for me, so I'd bet that this fix is good 👍 |
Travis failure on GHC 7.4.2 |
Connection could be closed via explicit `connectionClose` call or via finalizer attached to it. Both should check status to prevent writing to or reading from closed connection. See snoyberg#225
Thanks! |
It is fix for #225
It also contains related patch to cleanup test suit, please let me know if you want me to send it separately.