Npick fix

[ZackC]

#340 - removed the line that causes a list of pending promises to be canceled.

[ZackC]

From my understanding, the build checks fail because of these tests in ./test/core/test_lwt.ml:

(* The behavior of [p] tested here is a current bug in [Lwt.npick]. *)
test "npick: pending, resolves" begin fun () ->
let p1, _ = Lwt.task () in
let p2, r = Lwt.task () in
let p = Lwt.npick [p1; p2] in
assert (Lwt.state p = Lwt.Sleep);
Lwt.wakeup r "foo";
Lwt.return
(Lwt.state p1 = Lwt.Fail Lwt.Canceled &&
Lwt.state p = Lwt.Fail Lwt.Canceled)
end;

(* This is the same bug as above. *)
test "npick: pending, fails" begin fun () ->
let p1, _ = Lwt.task () in
let p2, r = Lwt.task () in
let p = Lwt.npick [p1; p2] in
Lwt.wakeup_exn r Exception;
Lwt.return
(Lwt.state p1 = Lwt.Fail Lwt.Canceled &&
Lwt.state p = Lwt.Fail Lwt.Canceled)
end;

These tests were testing for buggy behavior that is now fixed.

[ZackC]

New commit that moves the cancellation of promises to after some of them have the chance to complete.

[ZackC]

Huh, the build produced an unable to install parsetree error. That shouldn't be related to the changes I made.

[aantron]

Indeed it's not, see ocaml-ppx/ocaml-migrate-parsetree#19 (comment). We can just ignore that build row for now; I'll list in allowed_failures: later.

[aantron]

Ok, it's pretty much correct, but I'm asking for a change due to a pure subtlety. Sorry that things like that matter in Lwt. This is a learning process for me too...

[aantron]

I believe this reordering produces the correct behavior.

There is one weird thing about Lwt now, though:

• The callback of Lwt.pick cancels the pending promises before completing its own ("final") promise.
• This modified callback of Lwt.npick cancels the pending promises after completing its own promise.

This means that code running in on_cancel and regular callbacks of the respective pending promises can observe different states of the final promise depending on which function is used. Likewise, code running in the callbacks of the final promises can observe different states of the (to-be) canceled promises, depending on which function is used.

I'm not sure if that's really a problem, since we don't document or guarantee either behavior, and I'm not aware of any pathological interactions due to picking one or the other, so one could argue that this doesn't need to be addressed right now.

However, it could cause problems for users refactoring their code. For completeness, this could be addressed by factoring out the implicit final promise completion from finish_nchoose_or_npick_after_pending, so that function only collects the result, and the callback can fully control the order of (1) figuring out the result (2) canceling the pending promises (3) setting the state of the final promise (the one returned by npick itself). Callbacks in both npick and nchoose will have to be modified to call complete explicitly.

[aantron]

A secondary reason to prefer the ordering in pick is that function gets used a lot, so presumably that behavior is not problematic.

[aantron]

This is fine, but FYI OCaml supports nested comments, so this can be reverted.

[aantron]

Thanks for inserting this here!

[aantron]

@ZackC I wrote a small program to show the two behaviors, in part for my own education, but it may also be helpful for ensuring same ordering: https://gist.github.com/aantron/4db12b8e5ca92f57f7a223e594c833cc

[aantron]

Thanks for fixing this!

[ZackC]

No problem. Glad to help!