Skip to content

Conversation

mkroening
Copy link
Contributor

This PR makes embedded-io-async's Write::flush method a required method, meaning all implementors have to implement it.

This aligns this method with std's Write::flush and embedded-io's Write::flush.

This PR is more a question than a proposal.
I could not find any documented reasons for the async flush being a provided method.
If this turns out to be intentional, I am happy to leave it as is and document the reason instead.

CC: @Dirbaio, who added this in the initial design (d6f6419).

@mkroening mkroening requested a review from a team as a code owner September 4, 2025 08:52
@mkroening mkroening force-pushed the async-write-flush branch 2 times, most recently from ad2fb51 to aa3f838 Compare September 4, 2025 08:54
@mkroening mkroening changed the title feat: make async Write::flush a required method feat(io): make async Write::flush a required method Sep 24, 2025
@mkroening mkroening mentioned this pull request Sep 24, 2025
3 tasks
@chrysn
Copy link
Contributor

chrysn commented Sep 24, 2025

The only difference between providing a no-op and requiring it is in convenience to an implementer that doesn't buffer (who'd need to just put the empty fn in) and in ensuring everyone thinks about whether they do (making writing the empty impl fn a written statement of "we indeed don't need to flush").

This is not so much a technical but a educational / social question: Do we expect that people who buffer know that they need to implement a flush (because in their write impl, they explicitly chose not to really write), or do we want to rub it in visibly, even at the cost of annoying users who don't do buffering?

Personally, whenever I implemented this I wasn't buffering, so I was glad that the no-op is provided (and would even advocate we do the same in the non-async version). That may need some little text in the write docs that cautions against buffering without impl'ing flush.

@mkroening
Copy link
Contributor Author

mkroening commented Sep 25, 2025

I think aligning with std as close as possible would be best. Any opinionated deviation has the potential to be confusing and should at least have a good reason.

According to the crate description, the crate provides traits equivalent to their std counterpart. The two major deviations (Error and WouldBlock) are explained in the README.

Also, there are recent precedents to being more restrictive than strictly necessary to align with std (#690, #695).

This is not so much a technical but a educational / social question: Do we expect that people who buffer know that they need to implement a flush (because in their write impl, they explicitly chose not to really write), or do we want to rub it in visibly, even at the cost of annoying users who don't do buffering?

This question has been answered by the Rust team as well (rust-lang/rfcs#576 (comment)):

rust-lang/rust#21835 (comment)
Should flush return Ok by default? We may want to make an explicit decision about the default here (there hasn't been much discussion on the flushing topic)

I think @mahkoh is raising a good point here, and flush should be a required method (no default impl) to help ensure it has a sensible implementation.

After having researched this a bit more, I think making this a required method would be the right call. Deviating from it would require a good explanation, in my opinion.

@chrysn
Copy link
Contributor

chrysn commented Sep 25, 2025

Reviewing that background, I agree that requiring a flush is at least a consistent thing to do. I don't think that embedded devices should generally take the std interfaces verbatim, especially when the standard interfaces follow conventions that have historically grown (in particular, embedded-nal should not look like POSIX sockets), but in this concrete case, consistency is what the crate promises, and this PR provides that consistency here.

@mkroening mkroening changed the title feat(io): make async Write::flush a required method feat(async-io): make async Write::flush a required method Sep 26, 2025
Copy link
Contributor

@chrysn chrysn left a comment

Choose a reason for hiding this comment

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

With -async also being up for a 0.7 release, this has entered the critical path of #679 (comment).

The code is pretty self-explanatory, looks correct, and with the emerging consensus that this is the way, repeating my previous ACK in as formal as I can a way here.

@mkroening
Copy link
Contributor Author

I rebased on latest master (there were conflicts due to #704). :)

@Dirbaio Dirbaio added this pull request to the merge queue Sep 29, 2025
Merged via the queue into rust-embedded:master with commit 8c2b548 Sep 29, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants