-
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
Lwt_react.limit: fix redundant event occurrences according to documented semantics #566
Conversation
And tests fail since it's suggested that there should be two events per limiting thread, not one. So it should either be documented or be fixed, I think. |
The fix does look correct. Thanks! I suggest fixing the test. I believe it involves just signaling the condition one more time in the right place(as you are probably aware). I think there is still a race condition remaining, which may or may not be worth fixing at this point. |
Do you mean that in case of simultaneous events there could be more than one callbacks attached? I'm not sure if it is possible with react since mapped function will be called sequentially, am I wrong? I'll fix the tests. |
Thanks! I'll try to make a small program to demonstrate what I mean with the race condition, perhaps later today or tomorrow. |
Ok, here it is: let () =
let start_time = Unix.gettimeofday () in
let print_elapsed_time () =
Printf.printf "%.1f\n%!" (Unix.gettimeofday () -. start_time)
in
let underlying_event, push = Lwt_react.E.create () in
(* "Control": this works as expected if uncommented, printing 0.0, 1.0: *)
(*
underlying_event
|> Lwt_react.E.limit (fun () -> Lwt_unix.sleep 1.)
|> Lwt_react.E.map print_elapsed_time
|> ignore;
push ();
push ();
*)
(* ...but we are going to do this instead, which prints 0.0, 1.0, 1.0: *)
underlying_event
|> Lwt_react.E.limit (fun () ->
let p = Lwt_unix.sleep 1. in
Lwt.on_success p push;
p)
|> Lwt_react.E.map print_elapsed_time
|> ignore;
push ();
push ();
Lwt_main.run (Lwt_unix.sleep 2.)
(* ocamlfind opt -linkpkg -package lwt_react,lwt.unix race.ml && ./a.out *) That What happens here is that the On the first The second One second later, the limiter promise resolves. The first After that, the second This is clearly a separate issue, so perhaps I'll open one (or you are welcome to). |
possible fix for #565