Fix exception on connection errors in chrome. #761
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Is there a way to add a test for this? |
|
I don't think there are tests for cohttp-http-jsoo yet ? It's doable, but we'd need to depend on nodejs to run the tests, and have a mock server to answer requests - I've done this for internal projects and it works like a charm. I can look into it but can't guarantee the timeline, I have a lot on my plate internally right now =) I can also help if someone wants to tackle this. |
mseri
approved these changes
Mar 31, 2021
There was a problem hiding this comment.
Please add a line to the change log (add also a current section on top for it).
I think we can merge this and open an issue here for @mefyl to plan tests for the jsoo module, so we can keep track of it even if it will take some time
|
Good news 1: I took the time to write some cohttp-lwt-jsoo tests. I will open up a separate PR to introduce the cohttp-lwt-jsoo test. |
|
The failure is due to an action setup error, and I can't seem to find the "try again" button 🤔 |
mseri
reviewed
Apr 13, 2021
| responseText"); | ||
| respText ()) | ||
| (fun ab -> `ArrayBuffer ab) | ||
| if Js.Opt.return xml##.response == Js.null then |
There was a problem hiding this comment.
- Does this need to be done also at: https:/mirage/ocaml-cohttp/pull/761/files#diff-17a4a55959fc8c74371af346bb1da1f88718607b785b1f1ca651b3b226d3f3cfR311
- If so, can we extract this function instead of having so much duplication?
- Is there a way to test this?
There was a problem hiding this comment.
Sure, IIRC there's even a comment by @avsm there that it is duplicated.
There was a problem hiding this comment.
I gave it a try, the only difference is the async version wrapping the response in a Lwt.t, so we'd need to parameterize the whole thing over the "future" monad. But really the whole Make_client_sync and Make_client_async are 99% duplicated, a simple abstraction over that monad would factor it (someone has been veeeery lazy or in a hurry when writing this 😅).
I don't mind taking a look, but honestly that should be a different PR.
There was a problem hiding this comment.
Regarding the reproduction, I know of no easy way. AFAICT it's the internal XHR implementation of chrome, and it's not clear when to me why responseType is "arraybuffer", but response is null. It looks like a mild bug according to MDN, but since it's meant to be used from javascript and everything fits everywhere™ in that magical world, it's probably not much of an issue - and by that I mean not a bigger issue that the rest of the language and libraries.
I can't reproduce it myself, I just get that exception reported a few times a day from our app running in the wild, all I can say is that it's possible to reach this state with a fairly recent chrome (electron 12). I thought I had a reproduction in the js test suite, which prompted me to drop this PR, but I was empirically wrong it seems.
There was a problem hiding this comment.
Fair enough. Given how complicated is managing exceptions across ocaml and js, I think it makes sense to be a bit proactive here. We should probably add a LOG message that the call returned a null response though, so it does not come unnoticed if it happens by mistake
There was a problem hiding this comment.
I don't mind taking a look, but honestly that should be a different PR.
That's good for me. Thanks for putting in the effort!
mseri
reviewed
Apr 14, 2021
| (fun () -> `String (Js.string "")) | ||
| (fun s -> `String s) | ||
| in | ||
| if xhr_response_supported then |
There was a problem hiding this comment.
match xhr_response_supported with
| true when Js.Opt.return xml##.response = Js.null ->
Firebug.console##log (Js.string "XHR Response is null; using empty string");
`String (Js.string "")
| true ->
Js.Opt.case
(File.CoerceTo.arrayBuffer xml##.response)
(fun () -> Firebug.console##log
(Js.string
"XHR Response is not an arrayBuffer; using responseText");
respText ())
(fun ab -> `ArrayBuffer ab)
| _ -> respText ()This is a readability suggestion, do you think it would be better or worse than the nested ifs?
|
I think this is good to go in, I can make extra changes -- if needed -- separately |
mseri
added a commit
to mseri/opam-repository
that referenced
this pull request
Nov 5, 2021
…wt-unix, cohttp-lwt-jsoo and cohttp-async (4.1.0) CHANGES: - cohttp-lwt-unix: Adopt ocaml-conduit 5.0.0 (smorimoto mirage/ocaml-cohttp#787) - cohttp-mirage: fix deprecated fmt usage (tmcgilchrist mirage/ocaml-cohttp#783) - lwt_jsoo: Use logs for the warnings and document it (mseri mirage/ocaml-cohttp#776) - lwt: Use logs to warn users about leaked bodies and document it (mseri mirage/ocaml-cohttp#771) - lwt, lwt_unix: Improve use of logs and the documentation, fix bug in the Debug.enable_debug function (mseri mirage/ocaml-cohttp#772) - lwt_jsoo: Fix exception on connection errors in chrome (mefyl mirage/ocaml-cohttp#761) - lwt_jsoo: Fix `Lwt.wakeup_exn` `Invalid_arg` exception when a js stack overflow happens in the XHR completion handler (mefyl mirage/ocaml-cohttp#762). - lwt_jsoo: Add test suite (mefyl mirage/ocaml-cohttp#764).
mseri
added a commit
to mseri/opam-repository
that referenced
this pull request
Dec 15, 2021
…wt-unix, cohttp-lwt-jsoo and cohttp-async (5.0.0) CHANGES: - Cohttp.Header: new implementation (lyrm mirage/ocaml-cohttp#747) + New implementation of Header modules using an associative list instead of a map, with one major semantic change (function ```get```, see below), and some new functions (```clean_dup```, ```get_multi_concat```) + More Alcotest tests as well as fuzzing tests for this particular module. ### Purpose The new header implementation uses an associative list instead of a map to represent headers and is focused on predictability and intuitivity: except for some specific and documented functions, the headers are always kept in transmission order, which makes debugging easier and is also important for [RFC7230§3.2.2](https://tools.ietf.org/html/rfc7230#section-3.2.2) that states that multiple values of a header must be kept in order. Also, to get an intuitive function behaviour, no extra work to enforce RFCs is done by the basic functions. For example, RFC7230§3.2.2 requires that a sender does not send multiple values for a non list-value header. This particular rule could require the ```Header.add``` function to remove previous values of non-list-value headers, which means some changes of the headers would be out of control of the user. With the current implementation, an user has to actively call dedicated functions to enforce such RFCs (here ```Header.clean_dup```). ### Semantic changes Two functions have a semantic change : ```get``` and ```update```. #### get ```get``` was previously doing more than just returns the value associated to a key; it was also checking if the searched header could have multiple values: if not, the last value associated to the header was returned; otherwise, all the associated values were concatenated and returned. This semantics does not match the global idea behind the new header implementation, and would also be very inefficient. + The new ```get``` function only returns the last value associated to the searched header. + ```get_multi_concat``` function has been added to get a result similar to the previous ```get``` function. #### update ```update``` is a pretty new function (mirage/ocaml-cohttp#703) and changes are minor and related to ```get``` semantic changes. + ```update h k f``` is now modifying only the last occurrences of the header ```k``` instead of all its occurrences. + a new function ```update_all``` function has been added and work on all the occurrences of the updated header. ### New functions : + ```clean_dup``` enables the user to clean headers that follows the {{:https://tools.ietf.org/html/rfc7230#section-3.2.2} RFC7230§3.2.2} (no duplicate, except ```set-cookie```) + ```get_multi_concat``` has been added to get a result similar to the previous ```get``` function. - Cohttp.Header: performance improvement (mseri, anuragsoni mirage/ocaml-cohttp#778) **Breaking** the headers are no-longer lowercased when parsed, the headers key comparison is case insensitive instead. - cohttp-lwt-unix: Adopt ocaml-conduit 5.0.0 (smorimoto mirage/ocaml-cohttp#787) **Breaking** `Conduit_lwt_unix.connect`'s `ctx` param type chaged from `ctx` to `ctx Lazy.t` - cohttp-mirage: fix deprecated fmt usage (tmcgilchrist mirage/ocaml-cohttp#783) - lwt_jsoo: Use logs for the warnings and document it (mseri mirage/ocaml-cohttp#776) - lwt: Use logs to warn users about leaked bodies and document it (mseri mirage/ocaml-cohttp#771) - lwt, lwt_unix: Improve use of logs and the documentation, fix bug in the Debug.enable_debug function (mseri mirage/ocaml-cohttp#772) - lwt_jsoo: Fix exception on connection errors in chrome (mefyl mirage/ocaml-cohttp#761) - lwt_jsoo: Fix `Lwt.wakeup_exn` `Invalid_arg` exception when a js stack overflow happens in the XHR completion handler (mefyl mirage/ocaml-cohttp#762). - lwt_jsoo: Add test suite (mefyl mirage/ocaml-cohttp#764).
mseri
added a commit
to mseri/opam-repository
that referenced
this pull request
Dec 15, 2021
…wt-unix, cohttp-lwt-jsoo and cohttp-async (5.0.0) CHANGES: - Cohttp.Header: new implementation (lyrm mirage/ocaml-cohttp#747) + New implementation of Header modules using an associative list instead of a map, with one major semantic change (function ```get```, see below), and some new functions (```clean_dup```, ```get_multi_concat```) + More Alcotest tests as well as fuzzing tests for this particular module. ### Purpose The new header implementation uses an associative list instead of a map to represent headers and is focused on predictability and intuitivity: except for some specific and documented functions, the headers are always kept in transmission order, which makes debugging easier and is also important for [RFC7230§3.2.2](https://tools.ietf.org/html/rfc7230#section-3.2.2) that states that multiple values of a header must be kept in order. Also, to get an intuitive function behaviour, no extra work to enforce RFCs is done by the basic functions. For example, RFC7230§3.2.2 requires that a sender does not send multiple values for a non list-value header. This particular rule could require the ```Header.add``` function to remove previous values of non-list-value headers, which means some changes of the headers would be out of control of the user. With the current implementation, an user has to actively call dedicated functions to enforce such RFCs (here ```Header.clean_dup```). ### Semantic changes Two functions have a semantic change : ```get``` and ```update```. #### get ```get``` was previously doing more than just returns the value associated to a key; it was also checking if the searched header could have multiple values: if not, the last value associated to the header was returned; otherwise, all the associated values were concatenated and returned. This semantics does not match the global idea behind the new header implementation, and would also be very inefficient. + The new ```get``` function only returns the last value associated to the searched header. + ```get_multi_concat``` function has been added to get a result similar to the previous ```get``` function. #### update ```update``` is a pretty new function (mirage/ocaml-cohttp#703) and changes are minor and related to ```get``` semantic changes. + ```update h k f``` is now modifying only the last occurrences of the header ```k``` instead of all its occurrences. + a new function ```update_all``` function has been added and work on all the occurrences of the updated header. ### New functions : + ```clean_dup``` enables the user to clean headers that follows the {{:https://tools.ietf.org/html/rfc7230#section-3.2.2} RFC7230§3.2.2} (no duplicate, except ```set-cookie```) + ```get_multi_concat``` has been added to get a result similar to the previous ```get``` function. - Cohttp.Header: performance improvement (mseri, anuragsoni mirage/ocaml-cohttp#778) **Breaking** the headers are no-longer lowercased when parsed, the headers key comparison is case insensitive instead. - cohttp-lwt-unix: Adopt ocaml-conduit 5.0.0 (smorimoto mirage/ocaml-cohttp#787) **Breaking** `Conduit_lwt_unix.connect`'s `ctx` param type chaged from `ctx` to `ctx Lazy.t` - cohttp-mirage: fix deprecated fmt usage (tmcgilchrist mirage/ocaml-cohttp#783) - lwt_jsoo: Use logs for the warnings and document it (mseri mirage/ocaml-cohttp#776) - lwt: Use logs to warn users about leaked bodies and document it (mseri mirage/ocaml-cohttp#771) - lwt, lwt_unix: Improve use of logs and the documentation, fix bug in the Debug.enable_debug function (mseri mirage/ocaml-cohttp#772) - lwt_jsoo: Fix exception on connection errors in chrome (mefyl mirage/ocaml-cohttp#761) - lwt_jsoo: Fix `Lwt.wakeup_exn` `Invalid_arg` exception when a js stack overflow happens in the XHR completion handler (mefyl mirage/ocaml-cohttp#762). - lwt_jsoo: Add test suite (mefyl mirage/ocaml-cohttp#764).
mseri
added a commit
to mseri/opam-repository
that referenced
this pull request
Dec 17, 2021
…wt-unix, cohttp-lwt-jsoo and cohttp-async (5.0.0) CHANGES: - Cohttp.Header: new implementation (lyrm mirage/ocaml-cohttp#747) + New implementation of Header modules using an associative list instead of a map, with one major semantic change (function ```get```, see below), and some new functions (```clean_dup```, ```get_multi_concat```) + More Alcotest tests as well as fuzzing tests for this particular module. ### Purpose The new header implementation uses an associative list instead of a map to represent headers and is focused on predictability and intuitivity: except for some specific and documented functions, the headers are always kept in transmission order, which makes debugging easier and is also important for [RFC7230§3.2.2](https://tools.ietf.org/html/rfc7230#section-3.2.2) that states that multiple values of a header must be kept in order. Also, to get an intuitive function behaviour, no extra work to enforce RFCs is done by the basic functions. For example, RFC7230§3.2.2 requires that a sender does not send multiple values for a non list-value header. This particular rule could require the ```Header.add``` function to remove previous values of non-list-value headers, which means some changes of the headers would be out of control of the user. With the current implementation, an user has to actively call dedicated functions to enforce such RFCs (here ```Header.clean_dup```). ### Semantic changes Two functions have a semantic change : ```get``` and ```update```. #### get ```get``` was previously doing more than just returns the value associated to a key; it was also checking if the searched header could have multiple values: if not, the last value associated to the header was returned; otherwise, all the associated values were concatenated and returned. This semantics does not match the global idea behind the new header implementation, and would also be very inefficient. + The new ```get``` function only returns the last value associated to the searched header. + ```get_multi_concat``` function has been added to get a result similar to the previous ```get``` function. #### update ```update``` is a pretty new function (mirage/ocaml-cohttp#703) and changes are minor and related to ```get``` semantic changes. + ```update h k f``` is now modifying only the last occurrences of the header ```k``` instead of all its occurrences. + a new function ```update_all``` function has been added and work on all the occurrences of the updated header. ### New functions : + ```clean_dup``` enables the user to clean headers that follows the {{:https://tools.ietf.org/html/rfc7230#section-3.2.2} RFC7230§3.2.2} (no duplicate, except ```set-cookie```) + ```get_multi_concat``` has been added to get a result similar to the previous ```get``` function. - Cohttp.Header: performance improvement (mseri, anuragsoni mirage/ocaml-cohttp#778) **Breaking** the headers are no-longer lowercased when parsed, the headers key comparison is case insensitive instead. - cohttp-lwt-unix: Adopt ocaml-conduit 5.0.0 (smorimoto mirage/ocaml-cohttp#787) **Breaking** `Conduit_lwt_unix.connect`'s `ctx` param type chaged from `ctx` to `ctx Lazy.t` - cohttp-mirage: fix deprecated fmt usage (tmcgilchrist mirage/ocaml-cohttp#783) - lwt_jsoo: Use logs for the warnings and document it (mseri mirage/ocaml-cohttp#776) - lwt: Use logs to warn users about leaked bodies and document it (mseri mirage/ocaml-cohttp#771) - lwt, lwt_unix: Improve use of logs and the documentation, fix bug in the Debug.enable_debug function (mseri mirage/ocaml-cohttp#772) - lwt_jsoo: Fix exception on connection errors in chrome (mefyl mirage/ocaml-cohttp#761) - lwt_jsoo: Fix `Lwt.wakeup_exn` `Invalid_arg` exception when a js stack overflow happens in the XHR completion handler (mefyl mirage/ocaml-cohttp#762). - lwt_jsoo: Add test suite (mefyl mirage/ocaml-cohttp#764).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On chrome/nodejs (ie. v8), the response field of XHR requests is
nullwhen the requests fails, as opposed to the empty string on firefox. In that case the coercion would fail,respText ()is called assuming it's a string and disaster ensues.