comprehensive-rust
comprehensive-rust copied to clipboard
google
Make chat-client reads cancellation safe
AsyncBufReadExt::read_line() isn't cancellation safe. In practice, this doesn't matter much because we expect the client to be run from a terminal, which buffers by lines anyway. Since using the cancellation-safe approach is so simple, it seems worth fixing.
Related: This is a pretty common mistake, particularly with select!. I'm debating between adding (possibly in a separate PR):
- a speaker note on the
select!slide - a separate slide (in "Pitfalls") about cancellation (e.g. could mention explicit signaling)
@djmitche @rbehjati thoughts? (n.b. I'm OOO, back on June 6)
- a speaker note on the
select!slide- a separate slide (in "Pitfalls") about cancellation (e.g. could mention explicit signaling)
I would love to see an explanation for this — I see the diff here, but it's not clear to me how this fixes a problem :smile: You can certainly be done in a followup PR.
I would love to see an explanation for this — I see the diff here, but it's not clear to me how this fixes a problem 😄 You can certainly be done in a followup PR.
I wrote a quick draft in #716 .
For the purposes of this PR, the potential data loss is due to the combination of:
-
select!drops the futures of unmatched branches, and - read_line advances the reader but doesn't necessarily expose data in the buffer until it reads a newline.
There are a few options, but using the lines() method is the simplest one I know of.
@mauvealerts Can you also update the list of required APIs in the description of the exercise?
In particular this line: https://github.com/google/comprehensive-rust/blob/c0138a424fdd4ebf108544363791295a067ce93b/src/exercises/concurrency/chat-app.md?plain=1#L32
Instead of BufReader::read_line(), we want the API for Lines::next_line().
I included links to both AsyncBufReadExt::lines() and Lines::next_line() as a single list-item, since they're so closely related. If it's too cluttered, I agree Lines::next_line() is more relevant.
I included links to both
AsyncBufReadExt::lines()andLines::next_line()as a single list-item, since they're so closely related. If it's too cluttered, I agreeLines::next_line()is more relevant.
Thanks. I think it is better to keep just Lines::next_line(), to be consistent with the rest of the listed APIs. We have not listed all the APIs, but only the ones required for completing the code. I assume that people will be able to easily find the API for the given code.