Lwt.awaken: replace wakeup and wakeup_later #1055
Conversation
TODO: - documentation - tests
|
A bit early for a review I suppose, but the right spelling would be immediately, and you might want to use |
|
@MisterDA thanks! |
|
proposal: replace the make the type of the two functions distinct: I'm unsure about the type thing. Any comments suggestions ideas? |
|
proposal: make two distinct functions instead of one with parameter
The idea being: you can either prioritise the currently executing code and delay the resolving to when the scheduler runs or you can prioritise the resolving promise and you have to actuaslly pause the current promise This is a more radical change that makes the semantics clearer (one promise executes and then the scheduler kicks in, not both promise) but also backwards incompatible (you can't express |
|
Currently unhappy with the interface (the named parameter + constructor is too long). Alternatives:
I'm currently leaning towards this last possibility: This makes the default case very simple and the complicated cases explicit. |
|
An issue with delaying the resolution of the promise is that the double-wakening exception has no place to be raised at. It happens asyncly meaning it should be handled by the async exception handler which I'm not keen to place more responsibility on. Still might be necessary. |
src/core/lwt.mli
Outdated
| {{!t} promise} associated with {{!u} resolver} [r]. This triggers callbacks | ||
| attached to the promise. | ||
| (** [ordering] indicates a scheduling strategy for fullfilling or rejection of | ||
| promnises associated to resolvers. |
There was a problem hiding this comment.
| promnises associated to resolvers. | |
| promises associated to resolvers. |
src/core/lwt.mli
Outdated
| distinct ways: | ||
|
|
||
| - [Deferred]: Resolves the promise later, after the current code reaches a | ||
| pause or some I/O. This is often the behaviour you want. It makes the |
There was a problem hiding this comment.
The sentence got cut off here.
src/core/lwt.mli
Outdated
| (if !has_happened then | ||
| (* Using [Later] for [ordering] causes this branch to be taken *) | ||
| print_endline "Current code was prioritised over resolved promise" | ||
| else | ||
| (* Using [Nested] for [ordering] causes this branch to be taken *) | ||
| print_endline "Resolved promise code was prioritised over current"); |
There was a problem hiding this comment.
is it not the reverse way? !has_happened is only true once task1 is resolved.
src/core/lwt_mutex.ml
Outdated
| (* We do not use [~order:Dont_care] here to avoid a stack overflow | ||
| when unlocking a lot of threads. *) | ||
| Lwt.wakeup_later (Lwt_sequence.take_l m.waiters) () | ||
| Lwt.awaken ~order:Dont_care (Lwt_sequence.take_l m.waiters) () |
There was a problem hiding this comment.
The comment disagrees with the code
* master: (155 commits) tweaks Match let-operator example Add headings to make the sections linkable Fix docstring code syntax Use sequencing PPX in main module docstrings Adjust generic syntax Add back documentation on sequencing PPX also support type annotations at top-level more windows tests in CI add workflow to test failing windows build engine set/id test: add conditionals to only test what can be engine set/id test expose a way to identify currently-running engine remove `-fdiagnostics-color=always` to fix #1082 remove debugging-helper executable ppx: improve handling of value binding constraints fix ppx: correctly move constraint in `let%lwt x : t = …` expressions lwt_unix_stubs.c: fix races reported by TSan Fix spelling mistakes in documentation mark incompatibilities with prereleases ...
TODO: