Improve docs for Waker::noop and LocalWaker::noop

[ijackson]

• Add a warning about a likely misuse. (See my commit message for longer rationale.)
• Apply some probably-accidentally-omitted changes to LocalWaker's docs
• Add a comment about the clone-and-hack of the docs

I have used semantic linefeeds for the docs formatting.

[rustbot]

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[tgross35]

I am not too familiar with these areas of async, so probably best to let somebody else take a look.

r? libs

[workingjubilee]

This is pervasively true, I don't feel it's worth a comment? If you want to fix it, then I would prefer a macro that expands into a few doc attrs, so it's actually fixed.

[ijackson]

I agree that it's pervasively true. I have seen similar situations in many other places in the rust-lang/rust codebase, often with unintended divergence as we see here. I'm afraid I don't have the tuits to engage in a campaign of docs deduplication. I find it difficult to predict the style and taste preferences in the Rust Project. I feel someone closer to the core of the project should demonstrate how this ought to be done, and probably the libs team should (at least implicitly, by lazy consensus) approve the idiom. (The docs are not precisely identical, so the macro wouldn't be entirely straightforward.)

IMO the comment is very helpful, even with the remaining duplication, because it tells you precisely where you need to edit the clone-and-hack. If that comment had been in place, perhaps 6f8a944 wouldn't have contained the slip. It's fairly lightweight answer to the problem.

If you don't agree, or want to insist that I do substantially more work instead of this stopgap, I will remove these hunks from this MR branch.

[workingjubilee]

Hmm... I see. I tried to do the refactoring I originally suggested and found it more of a pain in the ass than I expected. As-is, this seems adequate. It makes me vaguely annoyed, which made me initially reluctant about it, but on reflection it is an annoyance with the preexisting state of things.

[kpreid]

This text is, in my opinion, either incorrect or not specific enough about the situation it is warning against.

Waker::noop() is a perfectly good way to implement executing a task in the case where it is an acceptable performance tradeoff, or suitable for the application, to poll the task every time through some sort of loop (or to poll the task exactly once, as in now_or_never()). No wakeups are “lost” because the task is unconditionally awake. This is, to my mind, the main use case of Waker::noop(): to implement async execution trivially when it isn't worth the complexity of implementing it better.

It can only cause lost wakeups if you mix polling the task this way with polling the task another way (which is not trivial to do, since it involves shared ownership of the task, or using noop() within a future combinator). I get the impression from #98286 (comment) that your use case is perhaps doing something like that, but the text in this change is talking about non-async functions only, not mixing polling from a non-async function and polling from an executor.

Could you rephrase this to be more specific about the situation you are warning against?

[ijackson]

Thanks. I'll see what I can do.

I came here to do this because in conversations with colleagues, some of whom are fairly new to Rust, especially async Rust, hadn't understood the implications.

I can see that there can be situations where wakers aren't important, particularly in an executor. I think using a noop waker in a future implementation, without juggling caller(s)' waker(s), is probably a bug. Am I wrong about that?

[kpreid]

I think using a noop waker in a future implementation, without juggling caller(s)' waker(s), is probably a bug. Am I wrong about that?

Yes, if you are writing a future combinator (that is, impl Future for... that delegates to other Futures), then you should usually either pass on the provided context's waker, or create your own delegating non-noop wakers (if you want to track which of several child futures woke).

But the text doesn't say “when you are implementing Future::poll”. It says “from within a non-async, non-poll, function”, which rules out that case. So, I am confused.

[ijackson]

since it involves shared ownership of the task, or using noop() within a future combinator

Right. That is the scenario I'm trying to warn about, particularly using noop within a futures combinator.

I have tried to improve the text to be more specific. What do you think now? (I've avoided the term "future combinator" since it seems rather abstruse, and the reader who is about to make the mistake I'm warning about may not realise that that's what they're writing.)

[kpreid]

What do you think now?

That’s specific enough that it no longer has the “vague scary warning” problem, yes. Further comments:

• I don't think the words “runtime-agnostic” are adding anything here; if the future has knowledge about the runtime then that potentially overrides everything about the conventional Future/Waker protocol, so it doesn't need to be called out here.

• It’s still possible to use a noop waker correctly in this scenario. In particular, the usual time a wrapper future is likely to be without Context is at construction, and futures are always polled once to start. So, if Foo wraps Bar and polls Bar with a noop waker when Foo is constructed, then that's fine as long as Foo also polls Bar with the propagated waker when Foo is polled for the first time. I don't have an idea for how to describe this sort of scenario concisely, though.

I've avoided the term "future combinator" since it seems rather abstruse

Yes, and pedantically, you can have a future that wraps other futures without it being purely a combinator, so this is more correct.

[traviscross]

I understand what, @ijackson, you're trying to do here, and I appreciate the intent. And so it kind of saddens me to be so negative on this change, but...

When implementing a runtime-agnostic future which wraps other future(s), do not use this function as a way to be able to call poll methods when you don't have your caller's Context available. Using a no-op waker for that purpose would cause wakeups to be lost: the inner future would store only the no-op waker, replacing the proper waker for the waiting task.

In addition to the good points that @kpreid raised, when I read this, I think to myself, "anyone who is able to parse and interpret those two sentences already knows the limitations without needing to be told."

That's what I think the nature of the problem is here. It seems harder to write an accessible and correct warning than it is to just write an accessible statement of what this does do.

[kpreid]

Actually, how about something like this? Expanding the current paragraph that says “This is mostly useful for writing tests…”, we could write:

This is mostly useful for writing tests that need a Context to poll some futures, but are not expecting those futures to wake the waker or do not need to do anything specific if it happens. More generally, using Waker::noop() to poll a future discards the notification of when the future should be polled again, and therefore, it should only be used when such a notification will not be needed to make progress.

I think this would fairly well get across some useful points:

• What you are doing here is discarding a message. (People understand that this can mean that progress stops because the message is not delivered where it is needed.)
• It is reasonable to discard a message specifically when nothing needs to be done about it.

without getting further into the details of precisely classifying what situations this applies to.

[ijackson]

I like your suggested text. Thank you! I will adopt it into the MR.

[traviscross]

While I understand the motivation here, I don't think these changes are an improvement.

We should carefully describe what the no-op waker does. We don't need such a strong warning here about what it doesn't do.

Maybe there is a way to say what's trying to be said here, but as it's written, it just feels "off" to me, and I'm not sure it's an important enough point to try to rework this.

[ijackson]

I've adopted @kpreid's suggestion, with some light edits. I hope folks like this better.

[workingjubilee]

Yes, I think this is much better as it informs more "positively" than originally framed, answering "What is this for?" It's when you don't care about the notification.

I'm good with this assuming @traviscross doesn't have any further comments, though I would appreciate it if you rebased the history to have less of the back-and-forth before we r+ it.

[traviscross]

As @workingjubilee said, this is a better framing. Thanks for the changes.

[kpreid]

I think this would read slightly better as a single paragraph, since the "More generally…" is directly referring to what was just introduced, whereas the paragraphs above and below this pair are much more unrelated.

Suggested change

///

[ijackson]

I think I prefer it in two paragraphs. The first paragraph is about tests; then the second paragraph is indeed more general.

I think if we join these together the order should be reversed, so that the general case, with the clear statement about discarded notifications, isn't buried in a paragraph which on first glance seems to be just about testing.

[kpreid]

(Matching above suggestion.)

Suggested change

///

[ijackson]

I would appreciate it if you rebased the history to have less of the back-and-forth before we r+ it.

Right. Done.

[workingjubilee]

Thanks!

@bors r+ rollup

[bors]

📌 Commit 9a95573 has been approved by workingjubilee

It is now in the queue for this repository.