Add needs_host_poll() and should_block_read() to IOPollable#753
Add needs_host_poll() and should_block_read() to IOPollable#753
Conversation
Add two new default methods to the IOPollable trait: - needs_host_poll(): indicates the pollable cannot deliver async observer notifications (e.g. host-backed stdin), so callers should poll periodically. - should_block_read(): indicates whether reads should block when no data is available. Returns false for fds that use epoll/poll and expect EAGAIN. Both default to the backward-compatible behavior (no host poll needed, reads block). Forward the methods through the Arc<T> blanket impl.
|
🤖 SemverChecks 🤖 No breaking API changes detected Note: this does not mean API is unchanged, or even that there are no breaking changes; simply, none of the detections triggered. |
jaybosamiya-ms
added
the
expmt:shadow-kiln
jaybosamiya-ms
left a comment
jaybosamiya-ms
left a comment
There was a problem hiding this comment.
Without the context of why these are needed, which I would expect to see in the PR description at least, it is unclear what these are adding. Neither the implementors nor the callers of this show up, so without context, this is simply increasing the interface size without clear indication of why.
Also, I am not convinced at least as is this is a good design. It is almost never the case that a boolean-returning design is the right move. In this particular case, maybe it is register_observer that should return a #[must_use] object that forces you to poll for the needs_host_poll state? I'm not even sure what the should_block_read is supposed to do here.
Importantly, without the context of the users/implementors of this interface it is not clear to me this is a good interface. And indeed since they are just boolean-returning things, they are essentially acting like config flags, which is more-often-than-not a bad interface design.
| /// implementors; callers arrive in subsequent stacked PRs. | ||
| fn should_block_read(&self) -> bool { | ||
| true | ||
| } |
There was a problem hiding this comment.
A few comments here:
- documentation comments should not talk about "existing implementors" and such; they are meant to be long-term objects in the codebase
- similarly, callers and other stacked PRs should not be in the docs
- Also see top-level comment for this review
2 participants
Summary
needs_host_poll()default method toIOPollable— returnsfalseby default; overridden by IO objects that require the host to perform the actual poll (e.g., host-backed stdin).should_block_read()default method toIOPollable— returnstrueby default; overridden by IO objects where reads should never block even when the fd is in blocking mode.Waker::ptr_eq()utility for comparing waker identity without waking.Split from #743.