Skip to content

Fix Invalid_arg exception on stack overflow when waking up a request. #762

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 2 commits into from
Apr 1, 2021
Merged

Fix Invalid_arg exception on stack overflow when waking up a request. #762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3162499
Fix Invalid_arg exception on stack overflow when waking a request.
mefyl Mar 28, 2021
023deb8
Update CHANGES.md.
mefyl Apr 1, 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
5 changes: 5 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## current

- lwt_jsoo: Fix `Lwt.wakeup_exn` `Invalid_arg` exception when a js
stack overflow happens in the XHR completion handler (@mefyl #762).

## v4.0.0 (2021-03-24)

- cohttp.response: fix malformed status header for custom status codes (@mseri @aalekseyev #752)
Expand Down
15 changes: 14 additions & 1 deletion 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 @@ -231,7 +231,20 @@ module Make_client_async (P : Params) = Make_api (struct
* Remove the type constraint on Lwt.task above and return any old
* guff here. It'll compile and crash in the browser! *)
Lwt.wakeup wake (response, body)
with e -> Lwt.wakeup_exn wake e)
with
| e
(* If we exhaust the stack, it is possible that
Lwt.wakeup just aboves marks the promise as
completed, but raises Stack_overflow while
running the promise callbacks. In this case
waking calling wakeup_exn on the already
completed promise would raise an Invalid_arg
exception, so although the promise is in a
really bad state we may as well let the actual
Stack_overflow exception go through. *)
when Lwt.state res = Lwt.Sleep
Copy link
Copy Markdown
Collaborator

@mseri mseri Apr 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What about other genuine exceptions that we would have raised otherwise?

Copy link
Copy Markdown
Contributor Author

@mefyl mefyl Apr 1, 2021

Choose a reason for hiding this comment

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

The exception doesn't really matter : if we end up in this state and Lwt.state res is not Sleep, calling Lwt.wakeup_exn state _ will raise Invalid_arg since you can't wake a promise twice, so we're a bit stuck between a rock and a hard place. In the end calling wakeup_exn will not wake anything and makes an Invalid_arg exception escape to the javascript toplevel, hiding the actual exception. This patch makes the actual exception, Stack_overflow, reach the toplevel instead, which I argue is always better, although not perfect.

In practice: I often get a Stack_overflow error in javascript, but I also had this Lwt.wakeup_exn: Invalid_arg exception sometime in production logs. After digging I saw this was actually a hidden stack overflow too, with this patch it is successfully reported as so without altering anything else.

Copy link
Copy Markdown
Collaborator

@mseri mseri Apr 1, 2021

Choose a reason for hiding this comment

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

I understand now, thanks. Then, indeed, solution seems reasonable and also my other comment makes less sense :P

->
Lwt.wakeup_exn wake e)
| _ -> ());

(* perform call *)
Expand Down