Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Clarify Read trait blocking behavior #625

Merged
merged 5 commits into from
Sep 9, 2024
Merged

Clarify Read trait blocking behavior #625

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b1e754
Clarify Read trait blocking behavior
ivmarkov Aug 24, 2024
45fcad0
Update embedded-io/src/lib.rs
ivmarkov Aug 24, 2024
92dbaf2
Re-add an unintentionally removed paragraph
ivmarkov Aug 24, 2024
f3558ac
Add identical changes to the async Read variant
ivmarkov Aug 26, 2024
d745418
Address review feedback
ivmarkov Sep 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions embedded-io/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,16 +300,23 @@ impl<E: fmt::Debug> std::error::Error for WriteFmtError<E> {}
pub trait Read: ErrorType {
/// Read some bytes from this source into the specified buffer, returning how many bytes were read.
///
/// If no bytes are currently available to read, this function blocks until at least one byte is available.
/// If no bytes are currently available to read:
/// - The method blocks until at least one byte becomes available;
/// - Once at least one (or more) bytes become available, a non-zero amount of those is copied to the
/// beginning of `buf`, and the amount is returned, *without waiting or blocking any further for
/// more bytes to become available*.
///
/// If bytes are available, a non-zero amount of bytes is read to the beginning of `buf`, and the amount
/// is returned. It is not guaranteed that *all* available bytes are returned, it is possible for the
/// implementation to read an amount of bytes less than `buf.len()` while there are more bytes immediately
/// available.
/// If bytes are available to read:
/// - A non-zero amount of bytes is read to the beginning of `buf`, and the amount is returned immediately,
/// *without blocking and waiting for more bytes to become available*;
/// - It is not guaranteed that *all* available bytes are returned, it is possible for the implementation to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit: this line looks like it applies only to the "if there are bytes available to read" case, while it should apply to both. Perhaps split it to a new paragraph and remove the -.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, but it is applicable only to the "if there are bytes available to read" case, as it talks about - if there are bytes available to read in the first place - how many of those would be returned - i.e. not necessarily all of them (because (a) the buffer might actually be shorter than what is available (b) because the implementation just decides so, for whatever other reasons).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This applies to the first case as well, doesn't it? In "Once at least one (or more) bytes become available", one could also ask if it's guaranteed that all available bytes are returned. The answer is "it is not guaranteed", but it's not completely obvious from the description as it is written now.

But as Dirbaio wrote, it is a "minor nit". Personally, I would not expect to have such a guarantee when reading the text you proposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how come the first case is unclear that only a subset of the just-read bytes are returned, given that it says (emphasis mine):
"

  • Once at least one (or more) bytes become available, a non-zero amount of those is copied to the beginning of buf, and the amount is returned...

"

"a non-zero amount of those". Do we really need to be more explicit than that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah be it. If two folks find it unclear then it is probably unclear. :(
I'll re-arrange so that the problematic text is a separate paragraph.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jannic I've re-arranged it now so that the problematic text is a separate paragraph. If you can take a quick look - thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how come the first case is unclear that only a subset of the just-read bytes are returned, given that it says (emphasis mine): "

* Once at least one (or more) bytes become available, **a non-zero amount of those** is copied to the beginning of `buf`, and the amount is returned...

"

"a non-zero amount of those". Do we really need to be more explicit than that?

IMHO the only possible source of confusion was the asymmetry between the two cases. Yes, in general I agree that the wording "a non-zero amount of those" was sufficiently clear. But then the second case, in addition to that, explicitly stated that it's allowed to return less than available. For the first case, that sentence was missing. Readers may wonder why? Is there a difference in guarantees?

Again, this was a very minor detail. But if we can avoid this possible confusion by just putting the last sentence in a separate paragraph, as you now did, we should do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, don't want to over-complicate it, but now - with the problematic text being a separate paragraph - the next paragraph that starts with "This waiting behavior is important for the cases where Read..." becomes unclear... in that readers might be confused that it only refers to the newly-born "Note..." paragraph, while it refers to everything mentioned above.

Any brilliant ideas? Or is it just my mind that finds the new arrangement also problematic?

/// read an amount of bytes less than `buf.len()` while there are more bytes immediately available.
///
/// If the reader is at end-of-file (EOF), `Ok(0)` is returned. There is no guarantee that a reader at EOF
ivmarkov marked this conversation as resolved.
Show resolved Hide resolved
/// will always be so in the future, for example a reader can stop being at EOF if another process appends
/// more bytes to the underlying file.
/// This blocking behavior is important for the cases where `Read` represents the "read" leg of a pipe-like
/// protocol (a socket, a pipe, a serial line etc.). The semantics is that the caller - by passing a non-empty
/// buffer - does expect _some_ data (one or more bytes) - but _not necessarily `buf.len()` or more bytes_ -
/// to become available, before the peer represented by `Read` would stop sending bytes due to
/// application-specific reasons (as in the peer waiting for a response to the data it had sent so far).
///
/// If `buf.len() == 0`, `read` returns without blocking, with either `Ok(0)` or an error.
/// The `Ok(0)` doesn't indicate EOF, unlike when called with a non-empty buffer.
Expand Down