Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Wake trait for safe construction of Wakers. #68700
Add Wake trait for safe construction of Wakers. #68700
Changes from all commits
06ede35
d8a835f
c9acdb0
ede03a4
3ae74ca
a4875a7
caff9f9
32f5724
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
futures::task::ArcWake
has the opposite defaulted method. Is there a reason to assume that implementations would commonly need to own theArc
instead of being able to wake through a shared reference?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.
In my experience working on romio/juliex and reading the tokio source, the standard executor model is to re-enqueue the task on some TLS or global queue (which means it must have ownership), and the standard reactor model is to store an
Option<Waker>
and then call.take().unwrap().wake()
. IMO the default in futures is backwards, and would result in naive implementations making an additional unnecessary ref count increment.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 have a strong opinion about this either way-- I can definitely imagine arguments on either side. The reason I did it the other way in futures-rs was that I imagined implementations which only needed by-reference waking would implement just
wake
not realizing that that would result in awake_by_ref
method which cloned unnecessarily. In practice, I don't think either case is too big of an issue.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.
Yea, either way I think its a chance of an incorrectly implemented executor leading to one extra incr/decr on wakeup - a trivial and easily fixable problem.
But I do think the more likely scenario is that they do need ownership of the arc, rather than that they don't (the only point of it being by arc is to cheaply enqueue on wake up after all), and I thought our choice to name the fns
wake
andwake_by_ref
was tied up in this assumption (that wake would be the default one).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.
If you use
ManuallyDrop
like inwake_by_ref
, can't the entire thing just becomeThere 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.
Personally I find this less clear than mem::forgetting the clone.
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.
Hm. I find it really confusing to create a clone only to forget it immediately. Conceptually speaking, you want to return the clone (that's what the function name says), but without dropping the original.
If you stick to the current code, I think this needs a comment explaining why it makes any sense to create a clone that is immediately forgotten.
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.
For me clone + mem forget an Arc is a pretty clear "increment the ref count" signal (TBH just having unsafe methods to fiddle with Arc's ref count would be nicer than any of this). What I would find non-obvious is why you need to put self into a manually drop.
There is a comment on this function indicating that it's purpose is to increment the refcount.