-
Notifications
You must be signed in to change notification settings - Fork 177
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
Handle uncaught exceptions in Lwt_react.of_stream. #809
Conversation
Hiya, I'm guessing you stumble onto this issue because the I have the following reservations:
|
@raphael-proust, regarding the last point, there are a few other places where we use |
@raphael-proust It's simpler than that. When triggering a reactive update due to setting a signal, event, or running 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 file:
|
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 |
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.
|
225e115
to
83e28d0
Compare
I agree, your phrasing is more clear. I pushed an update. |
LGTM. I'll merge after a bit in case someone else wants to review / comment. |
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.