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

Deprecate Lwt_main.yield and Lwt_unix.yield? #855

Closed
favonia opened this issue May 20, 2021 · 7 comments
Closed

Deprecate Lwt_main.yield and Lwt_unix.yield? #855

favonia opened this issue May 20, 2021 · 7 comments

Comments

@favonia
Copy link
Contributor

favonia commented May 20, 2021

This is a suggestion. After talking to @aantron on Discourse, I feel we should officially deprecate yield and related functions using [@@alert deprecate], in hope that one day we will merge the two pools into one.

@cyberhuman
Copy link

How do you yield then? 🤔

@favonia
Copy link
Contributor Author

favonia commented May 21, 2021

Lwt.pause () will do the job. There are currently two pools in the codebase due to the two almost identical mechanisms, but I believe they can be safely merged once every Lwt_unix.yield () and Lwt_main.yield () is replaced by Lwt.pause ().

@aantron
Copy link
Collaborator

aantron commented May 21, 2021

See

val yield : unit -> unit Lwt.t
(** [yield ()] is a pending promise that is fulfilled after Lwt finishes
calling all currently ready callbacks, i.e. it is fulfilled on the next
“tick.”
[yield] is similar to {!Lwt.pause} and it exists for historical reason.
Prefer [pause] in order to stay compatible with other execution
environments such as js_of_ocaml. *)

and https://discuss.ocaml.org/t/lwt-pause-versus-yield/7875.

@raphael-proust
Copy link
Collaborator

See #784 for some earlier discussion of this.

TL;DR:

  • Lwt_main.run treats paused and yielded promises differently: it resolves paused promises twice as often as yielded promises.
  • This is not an intended difference, but it's a significant difference and it may be significant for several binaries.

I think that, before we release this we should:

  • run the tests of all the opam packages that depend on lwt with the modified lwt (I think this is possible through the opam CI in some way)
  • notify packages that we break or even give them some patches
  • advertise the possibility of the change on discuss (and maybe other channels)

Then if all of that goes well, we can release. It'll need a "breaking change" mention in the changelog.

Maybe we could leverage the opam infrastructure to test if we break some of the tests of the opam packages. I think we should do this (if possible), make upstream commits to all possibly broken packages, and we should make a special mention of this in the changelog (and advertise it in advance on discuss too).

@favonia
Copy link
Contributor Author

favonia commented May 25, 2021

I just updated CHANGES in the #858 but need help in all other actionable items.

@raphael-proust
Copy link
Collaborator

I'll ask some opam wizards about the general test of all dependencies.

craigfe added a commit to craigfe/irmin that referenced this issue Nov 26, 2021
... by replacing them with the almost-equivalent `Lwt.pause`. Promises
constructed in this way have slightly different scheduling behaviour,
but this shouldn't impact the correctness of our code.

See ocsigen/lwt#855 for more details.
samoht pushed a commit to samoht/irmin that referenced this issue Nov 26, 2021
... by replacing them with the almost-equivalent `Lwt.pause`. Promises
constructed in this way have slightly different scheduling behaviour,
but this shouldn't impact the correctness of our code.

See ocsigen/lwt#855 for more details.

(cherry picked from commit 04a5bfa)
samoht pushed a commit to samoht/irmin that referenced this issue Nov 26, 2021
... by replacing them with the almost-equivalent `Lwt.pause`. Promises
constructed in this way have slightly different scheduling behaviour,
but this shouldn't impact the correctness of our code.

See ocsigen/lwt#855 for more details.

(cherry picked from commit 04a5bfa)
@favonia
Copy link
Contributor Author

favonia commented Nov 27, 2022

Closing this as #917 will complete every actionable items in this issue.

@favonia favonia closed this as completed Nov 27, 2022
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

No branches or pull requests

4 participants