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

client-ghcjs: Merged master + Fix MVar blocking exception #610

Closed
wants to merge 58 commits into from

Conversation

FPtje
Copy link
Contributor

@FPtje FPtje commented Oct 3, 2016

As discussed in #51 (comment) and #609.

This pull request contains a merge commit rather than a rebase. It also contains the fix for the following exception, which is thrown after every request:

thread blocked indefinitely in an MVar operation

Joseph Kachmar and others added 30 commits July 5, 2016 11:34
The Servant.JS Counter example was broken:

- `axios` was not completely applied. It now uses `defAxiosOptions`
similarly to the `angular` example
- `OverloadedStrings` was required, but not included
…rvant-js-docs

Add more JS documentation.
…rvant/haskell-servant#581

* Version bump because this changes the API for GetHeaders
…tructions

add 'stack setup' command to CONTRIBUTING.md
soenkehahn and others added 5 commits September 17, 2016 11:36
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
Copy link
Contributor Author

FPtje commented Oct 3, 2016

I pushed some commits removing redundant imports. Looks like your CI problem is solved too, @jkarni.

@alpmestan
Copy link
Contributor

Terrific, thanks!

@jkarni
Copy link
Member

jkarni commented Oct 4, 2016

Hmm, looking through the code, it's not obvious to me where or how GHCJS is being tested. @soenkehahn any insight?

In other news, stack does have binary distributions for GHCJS. Also, a trick I've seen in some GHCJS package (I can't remember which now) to deal the build timing out with cold caches is have a conditional check on the CI script to check whether the cache is cold. If so, only try building part of all the things, and then error out (someone would manually have to restart the build in those cases, or trigger another one with a commit).

@FPtje
Copy link
Contributor Author

FPtje commented Oct 4, 2016

I don't think ghcjs is being tested at all.

@jkarni
Copy link
Member

jkarni commented Oct 4, 2016

Ah. Sorry, maybe I wasn't clear. The CI problem I was thinking of was that we still didn't have a good way of testing GHCJS on CI.

@FPtje
Copy link
Contributor Author

FPtje commented Oct 4, 2016

I can imagine that being difficult, since you'd want to compile servant-client with both ghc and ghcjs and then perform tests with both of them.

@jkarni
Copy link
Member

jkarni commented Oct 4, 2016

Yeah... On the other hand, releasing something without CI into the wild sounds like a recipe for disaster.

We do have servers that we could use though. So we could have something that just runs a cronjob or handles a github hook, without starting from scratch every time. If you tell me how to run the tests locally, I could try to get that setup on my server.

@alpmestan
Copy link
Contributor

(Don't have time to check right now.) Wasn't there a library for facilitating the definition of github hooks?

@jkarni
Copy link
Member

jkarni commented Oct 4, 2016

@alpmestan yes, this.

Of course, we'd be running our own CI service, in essence. That's an unfortunate complication to CI, though if we simply can't get travis or Circle to cut it, one I'm okay with.

@FPtje
Copy link
Contributor Author

FPtje commented Oct 4, 2016

If travis can deal with ghcjs, it'll probably doable when the ghcjs client is separated from servant-client.

@jkarni
Copy link
Member

jkarni commented Oct 4, 2016

What do you mean separated? I thought from previous discussion the idea was a separate package (rather than a flag), but I imagined the codebase being the same.

@FPtje any chance you'll be in London for Haskell exchange? If so, we could try to figure out CI there together (@arianvp will also be there)

@FPtje
Copy link
Contributor Author

FPtje commented Oct 4, 2016

Yes, I mean having one servant-client package for the whole shebang with ghc, and a servant-client-ghcjs package purely for a ghcjs client.

@FPtje
Copy link
Contributor Author

FPtje commented Oct 4, 2016

Sadly, I won't be in London, by the way.

@jkarni
Copy link
Member

jkarni commented Oct 5, 2016

A few of us are in London, and are talking about this. On Saturday we're
going to sit down and try to get CI working, even if it's just a cron job
on my server.

Sorry this is all a bit slow and a bit of a drag, but thanks for picking it
up again!
On Oct 4, 2016 3:44 PM, "Falco Peijnenburg" notifications@github.com
wrote:

Sadly, I won't be in London, by the way.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#610 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABlKmjxYkwAeDHXty57TOSTMd6RTdmvuks5qwmZUgaJpZM4KMVhU
.

@jkarni
Copy link
Member

jkarni commented Oct 6, 2016

@dmjio pointed out https://docs.travis-ci.com/user/languages/nix - we could
try using that for a binary distribution of ghcjs...

On Wed, Oct 5, 2016 at 7:03 PM, Julian Arni jkarni@gmail.com wrote:

A few of us are in London, and are talking about this. On Saturday we're
going to sit down and try to get CI working, even if it's just a cron job
on my server.

Sorry this is all a bit slow and a bit of a drag, but thanks for picking
it up again!
On Oct 4, 2016 3:44 PM, "Falco Peijnenburg" notifications@github.com
wrote:

Sadly, I won't be in London, by the way.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#610 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABlKmjxYkwAeDHXty57TOSTMd6RTdmvuks5qwmZUgaJpZM4KMVhU
.

@arianvp
Copy link
Member

arianvp commented Oct 7, 2016

That looks cool. I did not know Travis could build nix expressions!

We already build nix expressions for all our repo's right? We can just adapt those to use ghcjs instead

@FPtje
Copy link
Contributor Author

FPtje commented Nov 6, 2016

Hey, I haven't heard from this for a while. Have there been any updates?

@arianvp
Copy link
Member

arianvp commented Jan 22, 2017

@FPtje , I got some more time to work on Servant again (scriptie was taking its toll ;P ), interested in grabbing a coffee soon?

@FPtje
Copy link
Contributor Author

FPtje commented Jan 22, 2017

@arianvp Sounds like a plan! I've sent you an email.

@jkarni
Copy link
Member

jkarni commented Sep 18, 2017

@FPtje #803 makes it easy to now implement servant-client-ghcjs as a separate library, which hopefully makes all of this much more pleasant. I'm closing this.

@jkarni jkarni closed this Sep 18, 2017
@FPtje FPtje mentioned this pull request Sep 24, 2017
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
main project
In Review
Development

Successfully merging this pull request may close these issues.

None yet