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

Handle uncaught exceptions in Lwt_react.of_stream. #809

Merged

Conversation

paurkedal
Copy link
Contributor

Currently exceptions raised during reactive updates set up by Lwt_react.of_stream will cause the event to stop without notice. My suggestion is to call !Lwt.async_exception_hook.

I ran into this while hunting down a bug in an Eliom application using Eliom_react.S.Down.of_react and with this PR I get the needed feedback in the JS console.

@raphael-proust
Copy link
Collaborator

Hiya,
Sorry for the long time it took for me to reply.

I'm guessing you stumble onto this issue because the stream you passed to of_stream got something pushed to it during the step that was triggered by a previous push. Can you share an example of this? Or an example of whatever caused the exception on your side?

I have the following reservations:

  • Major: the documentation of of_stream should explain what can lead to an exception being raised and possibly even give advice about how to avoid situations that lead to an exception being raised. This is why the example I'm asking about above is important. On this note, do we want to simply put a layer around the stream so that it delays popping elements until we are out of the current react step if any? And, would this prevent the exception you observed altogether?
  • Minor: Are there other places that this one where we lose exceptions? How can we find them?
  • Minor: async_exception_hook is supposed to be for handling exceptions that are raised in promises that have been ignored. It's not an exact fit in terms of intended use. OTOH, there isn't anything else and it's not too ugly of a design.

@aantron
Copy link
Collaborator

aantron commented Oct 5, 2020

@raphael-proust, regarding the last point, there are a few other places where we use Lwt.async_exception_hook when it's not directly related to promises. I believe one set is the Lwt_io.establish_server family of functions.

@paurkedal
Copy link
Contributor Author

@raphael-proust It's simpler than that. When triggering a reactive update due to setting a signal, event, or running Step.execute, the caller will receive exceptions raised during the reactive update. So, if we give a buggy function to React.E.map:

let () =
  let counter =
    let n = ref 10 in
    Lwt_stream.from
      (fun () -> Lwt.map (fun () -> decr n; Some !n) (Lwt_unix.sleep 0.2))
  in
  let e1 = Lwt_react.E.of_stream counter in
  let e2 = Lwt_react.E.map (fun i -> 1260 / i) e1 in
  let _e3 = Lwt_react.E.trace (Printf.printf "%d\n%!") e2 in
  let waiter, _ = Lwt.wait () in
  Lwt_main.run waiter

the current version will silently stop after printing 1260, while this PR we're told what is wrong:

$ dune exec ./test.exe
140                  
157
180
210
252
315
420
630
1260
Fatal error: exception Division_by_zero

Dune file:

(executable
 (name test)
 (modules test)
 (libraries lwt lwt_react lwt.unix))

@paurkedal
Copy link
Contributor Author

Do you still think the documentation is unclear? I think it hinges on the meaning of "updating the event", which refers to the execution of the reactive update step, which involve whatever the user has attached to the result of Lwt_react.E.of_stream.

@raphael-proust
Copy link
Collaborator

Oh I see, I wasn't familiar with React's policy of sending exceptions back to whichever called triggered the step.

I guess the documentation is ok. I'd maybe add something along the lines of the chunk below. But being unfamiliar with the exception-handling of React I might have written something that is immediately evident to React users. So if you feel like the added details are unnecessary just say so and we can go with your version.

        If updating the event causes an exception at any point during the
        update step, the excpetion is passed to
        [!]{!Lwt.async_exception_hook}, which terminates the process by
        default.

@paurkedal paurkedal force-pushed the lwt-react-of-stream-exception branch from 225e115 to 83e28d0 Compare October 7, 2020 20:19
@paurkedal
Copy link
Contributor Author

paurkedal commented Oct 7, 2020

I agree, your phrasing is more clear. I pushed an update.

@raphael-proust raphael-proust requested review from raphael-proust and removed request for raphael-proust October 8, 2020 05:55
@raphael-proust
Copy link
Collaborator

LGTM. I'll merge after a bit in case someone else wants to review / comment.

@raphael-proust raphael-proust merged commit 20466b8 into ocsigen:master Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants