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

fix MVar infinite blocking #425

Closed

Conversation

nrolland
Copy link

No description provided.

@@ -62,7 +62,9 @@ performXhr xhr request = do
callback <- onReadyStateChange xhr $ do
state <- readyState xhr
case state of
4 -> putMVar waiter ()
4 -> do
toPut <- isEmptyMVar waiter
Copy link
Member

Choose a reason for hiding this comment

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

A Race condition can still happen between this and putMVar. It won't actually fix the problem I think.

I think tryPutMVar is more appropriate. It's an atomic version of this behaviour.

Also, is it really a problem in the first place? I can not see a situation here where putMVar will actually block as waiter will always be empty before putMVar is called. (Because there is only one putMVar happening, namely on the onReadyStateChange callback)

@arianvp
Copy link
Member

arianvp commented Apr 23, 2016

@yogsototh Could you provide a bit more info about this pull request? I'd love to get the issue you're experiencing resolved.

@yogsototh
Copy link
Contributor

It was some time ago. But I made a repository to show the bug.

https://github.com/yogsototh/ghcjs-servant-client-bug

In my memory there were a runtime crash when waiting for some answer. At least using Safari. The demo should be a minimal form triggering a POST request.

I'm on mobile right now. I'll could give you more details later if you want.

@yogsototh
Copy link
Contributor

Ah yes, the function was called many time with the same state 4.

@arianvp
Copy link
Member

arianvp commented Apr 23, 2016

Also cc @soenkehahn as he was working on this branch

@arianvp
Copy link
Member

arianvp commented Apr 29, 2016

I will have a look at this this weekend.

@soenkehahn
Copy link
Contributor

@nrolland: I'd like to delete the branch client-ghcjs-wip in favor of client-ghcjs (https://github.com/haskell-servant/servant/tree/client-ghcjs). Could you please point this PR at the new branch?

@soenkehahn soenkehahn mentioned this pull request May 17, 2016
@soenkehahn
Copy link
Contributor

Sorry, closing this to be able to delete the client-ghcjs-wip branch.

@soenkehahn soenkehahn closed this May 23, 2016
FPtje pushed a commit to LumiGuide/servant that referenced this pull request Sep 30, 2016
Uses the fix mentioned by @arianvp in
haskell-servant#425.

See also
haskell-servant#51 (comment)

The cause of the issue very is similar to the issue described
in this blog post:
https://www.fpcomplete.com/blog/2016/06/async-exceptions-stm-deadlocks

The main thread creates a waiter, creates an asynchronous callback which is
called when the `readyState` of the request changes. When the readyState
changes to 4, which means 'request finished', the waiter MVar is put.
The main thread takes the MVar and continues to do stuff with the response.

The problem is that the `readyState` can change to 4 more than once,
for some reason. The second time this happens, the waiter MVar can be put
again, since the main thread took it. The main thread, however, won't take
it again. After all, it only needed to take the MVar once to know that the
request was finished. The third time `readyState` is set to 4, the putMVar
would block, causing the following exception to be thrown:

```
thread blocked indefinitely in an MVar operation
```

Since state 4 should really mean that the response is ready, it seems
appropriate to decide that all changes of the state to 4 after the initial one
can be safely ignored.
FPtje pushed a commit to LumiGuide/servant that referenced this pull request Oct 2, 2016
Uses the fix mentioned by @arianvp in
haskell-servant#425.

See also
haskell-servant#51 (comment)

The cause of the issue very is similar to the issue described
in this blog post:
https://www.fpcomplete.com/blog/2016/06/async-exceptions-stm-deadlocks

The main thread creates a waiter, creates an asynchronous callback which is
called when the `readyState` of the request changes. When the readyState
changes to 4, which means 'request finished', the waiter MVar is put.
The main thread takes the MVar and continues to do stuff with the response.

The problem is that the `readyState` can change to 4 more than once,
for some reason. The second time this happens, the waiter MVar can be put
again, since the main thread took it. The main thread, however, won't take
it again. After all, it only needed to take the MVar once to know that the
request was finished. The third time `readyState` is set to 4, the putMVar
would block, causing the following exception to be thrown:

```
thread blocked indefinitely in an MVar operation
```

Since state 4 should really mean that the response is ready, it seems
appropriate to decide that all changes of the state to 4 after the initial one
can be safely ignored.
waern pushed a commit to waern/servant that referenced this pull request Jan 5, 2017
Uses the fix mentioned by @arianvp in
haskell-servant#425.

See also
haskell-servant#51 (comment)

The cause of the issue very is similar to the issue described
in this blog post:
https://www.fpcomplete.com/blog/2016/06/async-exceptions-stm-deadlocks

The main thread creates a waiter, creates an asynchronous callback which is
called when the `readyState` of the request changes. When the readyState
changes to 4, which means 'request finished', the waiter MVar is put.
The main thread takes the MVar and continues to do stuff with the response.

The problem is that the `readyState` can change to 4 more than once,
for some reason. The second time this happens, the waiter MVar can be put
again, since the main thread took it. The main thread, however, won't take
it again. After all, it only needed to take the MVar once to know that the
request was finished. The third time `readyState` is set to 4, the putMVar
would block, causing the following exception to be thrown:

```
thread blocked indefinitely in an MVar operation
```

Since state 4 should really mean that the response is ready, it seems
appropriate to decide that all changes of the state to 4 after the initial one
can be safely ignored.
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.

4 participants