-
Notifications
You must be signed in to change notification settings - Fork 176
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
Remove obsolete second wakeup_paused #917
Conversation
If I remember correctly, the backend-agnostic/unix-free |
With this change, the behaviour of We could even remove |
There needs to be an entry in the CHANGES file. I can add it later before merging if you prefer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I don't know how to use github very well and I mostly had comments not related to the chunks anyway, so I left comments but apparently they don't count as a review but I think this does?)
While preparing a CHANGES entry I found this:
Now I'm wondering in what way existing applications could depend on this behaviour? |
Well for example, in your case (#916) you had to use three calls to |
Yes, resolving paused promises more often may break stuff intending to wait for one whole iteration. But resolving less often? No I don't think so. |
added a CHANGES entry |
Hi, I've added a commit to your branch. We can roll it back or amend it or something. It's more of a proposal/suggestion than anything. The idea behind it is that pause is now equivalent to yield (modulo the fact that yielded promises are resolved after which paused promises are. (So
So the commit just makes Let me know what you think. |
I think this is sensible. There is indeed no relevant difference between yield and pause. |
83ddb0b
to
d835010
Compare
github's UI is confisuin and Idk how to use reviews so I'm just dismissing this
It was introduced in d5822af. If I understand it correctly this was done to prevent calling select without timeout while there were pending paused promises. Since now we explicitely check for this condition and call select with zero timeout (`should_block_waiting_for_io`). This extra `wakeup_paused` seems to be obsolete.
It was introduced in d5822af.
If I understand correctly this was done to prevent calling select
without timeout while there were pending paused promises.
Since now we explicitely check for this condition and call select with
zero timeout (
should_block_waiting_for_io
). This extrawakeup_paused
seems to be obsolete.
Maybe in
Lwt.pause
it should also be documented that Lwt.pause () needs to be used 2 / 3 times to enforce completion of a whole main loop iteration. The current documentation talks about the 'next "tick"'? #916