Support GHCJS #51
Comments
|
With the change in the error type this got harder in 0.4. The |
|
Could you expand on why this got harder? |
|
|
|
Do you think we can make that bit be conditionally compiled when the compiler is not ghcjs? |
|
I would like to avoid that. The issue here is, that any client-side pattern matches would need the same annotations. I think it would be better to replace the exception with a simplified datatype of our own. That could then be populated with appropriate errors from either |
|
Do you feel like putting a PR together? |
|
Can do, but I'd like some input on the granularity. On actually making GHCJS work: I would probably user |
|
Some development related to this is happening at https://github.com/plow-technologies/ghcjs-servant-client |
|
I just pushed a new branch here: https://github.com/haskell-servant/servant/tree/client-ghcjs And some explanation about it here: https://github.com/haskell-servant/servant/blob/client-ghcjs/GHCJS.md I'm not planning to put that much more work into this, but I hope that this branch may serve as a basis for other people to improve the support for |
|
Thanks for this. In the next few weeks I may try to dig deeper and fix some of the problems you've outlined in the branch readme.md, but most likely only with respect to the failing test case. I don't feel like playing with javascript enough to tackle the json parsing issue. There are many branches with 'gchjs-wip" in the name, and I was wondering, for clarity, if you could delete the branches you think are fully unnecessary. I think as ghcjs and servant become more widespread as haskell development moves to the frontend, this effort will become increasingly more relevant. Thanks for all the effort put in to Servant and trying to get it to work with ghcjs, I'm sure there are many others with as much appreciation as myself. Looking forward to seeing Servant and ghcjs become more compatible. |
|
@soenkehahn can surely help in clarifying which branches are of interest. |
|
The branch linked above: https://github.com/haskell-servant/servant/tree/client-ghcjs. I've also tried to delete all |
|
@soenkehahn Are the only problems with the ghcjs branch listed in the GHCJS.md, or are they just the issues you are aware of? I was thinking about the xhr2 request discarding the request bodies, and was wondering if servant-client-ghcjs wants to adhere to that specification. It might be natural to try to adhere to the xhr2 specification and disallow request bodies for GET and HEAD when compiling with GHCJS. Regarding CI with the ghcjs branch, what in particular is difficult? You said you tried and failed miserably, but if I want to continue where you left off, what should I be looking out for regarding a potential pull request against master? |
@soenkehahn is the person with the more authoritative answer, but from what he said, part of the problem is requiring three (two GHCs, and one GHCJS) compilers, and not being able to cache builds due to the cache blowing up. Note that testing for the GHCJS branch requires building servers with GHC so that the client can be run against some server. |
|
@tdietert: There's also this issue that isn't fixed yet: #425. Apart from that this hasn't been tested in practice. So I wouldn't be surprised if there were more problems once you start using this branch for real.
Yes and no. I guess it makes sense to not send request bodies for But I'm not strictly against not supporting Regarding CI: I tried different things:
I next thing I would try (if I were to work on this) is coming up with a binary distribution for |
|
Looking at the most recent HTTP specification, it says this: "In particular, the convention has been established that the GET and HEAD methods SHOULD NOT have the significance of taking an action other than retrieval. These methods ought to be considered "safe". This allows user agents to represent other methods, such as POST, PUT and DELETE, in a special way, so that the user is made aware of the fact that a possibly unsafe action is being requested." With that in the specification, as well as xhr2 not supporting request bodies for GET and HEAD, it seems to me that it is essentially a minor violation of the HTTP protocol, and I'm wondering why Network.HTTP.Client supports this behavior to begin with. Perhaps it was too annoying to write the low-level haskell code to do this, as Network.HTTP.Client.Core seems to ignore request methods throughout its entirety. Would it be feasible using conditional compilation to disallow APIs that specify Request Bodies in GET and HEAD requests when compiling with GHCJS? I'm not sure what that would entail yet, but if you gave me the goahead I would look into it and try implementing that. We could also have conditional compilation for tests and just ignore the failing test (GET Request with Request body), or rely purely on QueryParams for transmission of data to server during GET and HEAD reqs... but I don't know, and I'm not really sure what kind of challenge this would be. @soenkehahn @jkarni @arianvp Does anyone have thoughts on this? I don't really see any other way, since xhr2 simply automatically discards the body of GET and HEAD reqs. |
|
I don't care much about whether request bodies in GET are discarded by the client or not. From what I remember from reading the RFCs a while back, it was not a violation of RFCs for a client to send a request body, but it was a violation for a server to do anything with it (e.g., RFC 2616, section 4.3). So for standard-compliant servers, it's not like it matters what the client does. I'm a little confused by the discussion of side-effects, though. Unless I'm misunderstanding, this seems completely orthogonal to whether request bodies are allowed in GETs. |
|
I don't agree that it's completely orthogonal, since if you have a request body in a GET, then naturally the server is able to do something with it or perform a side-effectful action, which is reserved for POST. I have removed that part from the comment above, but I think it's slightly relevant in the sense that, if the server should not do anything with the request body, why have the option for it? As it is, users of servant-client and servant-server can and most likely will use request bodies of GET requests in their server, when it is functionally allowed. In both ajax and xmlhttprequests, the GET and HEAD req bodies are ignored, and we are using ghcjs to compile haskell code to javascript, so it doesn't really make sense to allow request bodies in those request types, if by specification the server is not supposed to do anything with them. Both the most recent ajax and xmlhttprequests implement this by treating the request body as null before actually performing the request, so why should servant-client-ghcjs work any differently? I ask this genuinely, and am not sure why my initial suggestion was not sensible or why you thought the discussion of side effects based upon request bodies was unrelated, but I am not fluent in frontend web dev so I recognize I may be a bit naive. |
|
To be clear, I'm completely okay with the body being thrown out - we should probably even statically ensure that But the existence of arguments (specifically in the body) isn't really related to the presence of side effects. |
|
Ok, I understand. I am not well versed in the semantics of network request types but I'm always learning; I understand the distinction a bit better now. Would it be worth trying to statically ensure that ReqBody's don't appear in GET api endpoints and for it to be a type error? If so, I think I will try sometime in the next few days. That, along with a small modification to the test that @soenkehahn is concerned about failing so that ReqBodys are actually disallowed. I'm also going to look into PR #425 and see if the initial fix suggested works well enough. |
I would say so, though I think that should be a separate task than this, since it's both more general than, and not needed for, ghcjs. Ideally, for GHC 8, we would also have a nice type error message describing the reason for the type error. |
|
Do you mean different branch or just done in it's own pull request with respect to this branch? I like the idea of throwing out ReqBodys for all Thanks for all your help, I'll try to knock this out sometime in the next week or if I can make enough time. |
|
@tdietert: I'm not really actively working on this branch, so don't expect updates from my side. If you (or anyone else) wants to work on this, you could:
|
|
GET requestbodies MUST be ignored according to the HTTP RFC iirc. So i On Fri, 5 Aug 2016, 14:20 Thomas Dietert, notifications@github.com wrote:
|
|
I've been playing around with this branch, trying to apply it to a real world application. Since everything I've got to say is about the client-ghcjs branch of this repository, I've decided to post everything in this issue. ForkThere's a fork of the client-ghcjs branch which I updated to servant-0.9 (the latest master commit, not the tag). It just adds a single merge commit. Should I make a PR for this? I'm not sure whether a rebase is preferred over a merge commit. The merge was non-trivial. Implementation exampleThere's a repository containing a simple working example that shows some of the problems I've experienced with servant-client for ghcjs. It contains two test clients: one (called I will be referring to this working example in the rest of this post. Get requests with bodies
That's a question, and the answer is simply no. It doesn't work in browsers. I've tested it with both Chromium and Firefox. The working example has a get request route with a body. The server simply returns whatever it gets. In the end the request is performed, but the request body is discarded before the request is actually sent. The server thus both receives and then responds with an empty string. This behaviour can be reproduced with both Thread blocked indefinitelyAbout a second after performing a request, the following exception is thrown in the browser's console: It does not seem to affect functionality, but I'd recommend addressing it nonetheless. Currently, FormURLEncodedOne big change in This error looks very nasty. I'm not sure what precisely causes it or whether the responsibility of this error lies with Separate package?This is purely my own opinion, but wouldn't it be better if
FinallyDespite the things mentioned above, |
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.
|
I looked into the "Thread blocked indefinitely" exception and found it to be the same as the issue described by @yogsototh and @nrolland in #425. I ended up implementing @arianvp's suggestion after figuring out what was really going on. A full description of the issue and its solution can be found in the commit in my fork. Also, I verified that the callback does indeed get called with state 4 multiple times in a row with debug prints. |
|
Nice to see lumiguide working on this :). And nice to hear it works so well. Id prefer a rebased version. If you check out git-rerere it's probably as simple as an explicit merge commit . Anyhow. The argument to use a separate package is pretty strong due to the fact that we don't need manager. I think I would prefer this. You have plenty of valid arguments to this being the right choice. |
|
The primop error is a ghcjs problem. Pinging @luite |
|
@FPtje let's create an issue on the ghcjs repo. Also could you check the ghcjs 8.0.1 branch? |
|
@FPtje thanks for the work and write-up! The What about CI? This was, so far, the main issue we've had with merging the work onto master. |
|
If the issue lies with ghcjs it's probably worth mentioning that I've been using |
|
I've narrowed the primop exception down to a function in the |
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.
|
Alright, the PR has been made. I took a quick look at the CI problem. The job for the This is a documented error. It's described in this issue. The issue is closed as fixed in |
|
Looks like I missed a primop in the list. It doesn't look fundamentally hard to implement. Also I think ultimately these safe-haskell inconsistencies shouldn't be there, between GHC and GHCJS. I'm not sure how easy it'd be to fix them though. |
|
@jkarni, @FPtje: If I were working on this I would definitely go for a separate package for I've also entertained the idea of having a package that gives us pure request stubs for any client side use of |
|
+1 for the separate package. |
|
@soenkehahn, @jkarni, @FPtje, @alpmestan If we did want to go the single package route in @luite has mentioned this, and So the cabal file could look like: library
exposed-modules:
Servant.Client
ghc-options: -Wall
hs-source-dirs: src
build-depends:
base > 4.7 && < 5.0
, servant
, bytestring
, text
if impl(ghcjs)
hs-source-dirs: ghcjs-src
build-depends: ghcjs-base
else
hs-source-dirs: ghc-src
build-depends: aeson, http-clientSo it would look like this in the end Stack and cabal, as long as they're configured correctly, should handle it no problem. For nix expressions there would need to be some kind of conditional to bring the right deps in. { mkDerivation, aeson, base, stdenv, http-client, bytestring
, text, compiler, nixpkgs
}:
let
deps =
if compiler == "ghcjs"
then
let
ghcjs-base = nixpkgs.haskell.packages.ghcjs.ghcjs-base;
in
[ base servant bytestring text ghcjs-base ]
else
[ base servant bytestring text aeson http-client ];
in mkDerivation {
pname = "servant-client";
version = "0.9.0.1";
src = ./.;
libraryHaskellDepends = deps;
license = stdenv.lib.licenses.bsd3;
} |
|
What do you think about @dmjio's suggestion? I think it makes sense to share as much as possible in one package. |
|
Sounds like a good plan. |
|
+1 for package sharing, but there's still other issues, like supporting more than JSON, needing to make types for Blob / ArrayBuffer storage (and ContentTypes for these things). Since requests are asynchronous, we'll need a way to synchronize the sending thread with the receiving one (Maybe with clever usage of MVar). Lastly, Request bodies aren't supported on |
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.
|
I feel a bit silly for asking, but @FPtje said that the 'manager' argument gets ignored. When I try to run servant-client with GHCJS and servant-client-0.10, I get a runtime error when I pass in Am I missing something here? |
|
The PR I made never got merged. It was for servant 0.9. How did you get it to work on servant 0.10? But yeah, in my version the http manager gets ignored, as the browser takes over the task of managing http requests. In the ghcjs implementation the reference to the http manager simply remains unused. |
|
I'm going to call this done now that there's |
this is a good starting point.
The text was updated successfully, but these errors were encountered: