More stream reading improvements #999
Conversation
Be more precise about discarding all buffered state for stopped streams.
| if !self.stopped { | ||
| self.assembler.insert(frame.offset, frame.data); | ||
| } else { | ||
| self.assembler.set_bytes_read(end); |
There was a problem hiding this comment.
I think we actually had a bug where this (or equivalent) wasn't being done previously.
There was a problem hiding this comment.
That's right, this is a bit of a behavior change. Would you like me to split it out of this commit?
| /// all available data has been read. `max_length` may be used to restrict the maximum size | ||
| /// of the returned `Bytes` (but `usize::MAX` is supported, too). | ||
| /// | ||
| /// Will keep returning `None` once no further data is available for reading. |
There was a problem hiding this comment.
Outdated references to None
| if let Some(status) = &self.fused { | ||
| return ReadResult::from_status(*status); | ||
| } |
| let mut rs = match streams.recv.remove(&id) { | ||
| Some(rs) => rs, | ||
| None => return Err(ReadableError::UnknownStream), | ||
| }; |
There was a problem hiding this comment.
Are we missing a check for whether the stream is stopped here?
| /// The stream has been stopped | ||
| Closed, |
There was a problem hiding this comment.
This looks redundant to the UnknownStream case in ReadableError; a stream can never become stopped mid-read. It's also confusingly named.
quinn-proto/src/tests/mod.rs
Outdated
|
|
||
| let result = chunks.next(usize::MAX); | ||
| assert_eq!(result.status, ReadStatus::Finished); | ||
| assert_eq!(result.chunk, None); |
There was a problem hiding this comment.
Could we just check the status of the prior call, rather than calling again?
quinn/src/streams.rs
Outdated
| .inner | ||
| .read(stream, true)? | ||
| .next(buf.remaining()) | ||
| .map(|chunk| buf.put_slice(&chunk.bytes))) |
There was a problem hiding this comment.
To prevent loss of specific errors, we must check the status, stash any terminal states locally, and generate a suitable error without calling inner.read on the next poll. Similarly below.
|
Looks like there'll be a bit of a rebase hazard with #1000. Shouldn't be disastrous, though. |
|
I would be fine with waiting until this PR is merged and then rebasing my changes on top of it. |
|
Scoped this down again to remove the hard stuff (last commit) -- all the other comments should be addressed, so we can just get the simple stuff merged. @geieredgar thanks for being accomodating, I hope the rebase isn't too painful. |
|
(I will take review comments for the last commit in consideration for the new PR as well, of course.) |
No description provided.