Skip to content

Fix exception on connection errors in chrome. #761

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.

Sign up for GitHub

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

Merged
mseri merged 5 commits into from
Apr 14, 2021
Merged

Fix exception on connection errors in chrome. #761

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
22ead7a
Fix exception on connection errors in chrome.
mefyl Feb 14, 2021
048011e
Merge master into bugfix/null-response-chrome.
mefyl Apr 13, 2021
b09155c
Also fix null response bug in sync client.
mefyl Apr 13, 2021
658e67e
cohttp-lwt-jsoo: Factor body construction.
mefyl Apr 14, 2021
115173a
Add log when replacing null response with the empty string.
mefyl Apr 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions cohttp-lwt-jsoo/src/cohttp_lwt_jsoo.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,15 +202,18 @@ module Make_client_async (P : Params) = Make_api (struct
(fun s -> `String s)
in
if xhr_response_supported then
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)
if Js.Opt.return xml##.response == Js.null then

@mseri mseri Apr 13, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@mefyl mefyl Apr 13, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, IIRC there's even a comment by @avsm there that it is duplicated.

@mefyl mefyl Apr 13, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@mefyl mefyl Apr 13, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@mseri mseri Apr 13, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

@mseri mseri Apr 13, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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!

@mefyl mefyl Apr 14, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

`String (Js.string "")
else
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)
else respText ()
in
Bb.get b
Expand Down