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.

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

util: makes Framed and FramedStream resumable after eof #3272

Merged
merged 19 commits into from
Mar 22, 2021

Conversation

somethingelseentirely
Copy link
Contributor

@somethingelseentirely somethingelseentirely commented Dec 14, 2020

Motivation

As described in #3252 there are cases where underlying data sources might only temporarily return 0 bytes/ eof.
Framed and FramedRead can now resume reading such sources even after an initial eof.

This PR also prevents unexpected calls to decode_eof when frames are read after the frame stream has returned None.

For my use case of reading an append only logfile, I am now able to read frames until an an eof,
and then wait for an IPC notification on more available data.

PR also improves the documentation a bit by adding a general example for Framed.

Solution

The previous implementation had the assumption that 0 bytes returned from the underlying implementation implies that it will always do so. I've reused the existing state flags, and changed the control flow so that the the following 4 states are now possible.

name eof is_readable note default
reading false false tries to read from source into the buffer, if no bytes were read it goes to pausing, if bytes were available it goes to framing
framing false true repeatedly tries to decode a frame from the buffer using decoder.decode, staying in this state until unsuccessful, in which case it will go to reading
pausing true true handles the emitting of closing frames via decoder.decode_eof until the decoder declares no more closing frames through None, after which it goes to paused
paused true false similar to reading, in that it attempts to load more data into the buffer when called, except that reading no bytes directly returns None, thus staying in the paused state without emitting any closing frames, goes to framing on successful read
                             `decode_eof`                                
                             returns `Some`                  read 0 bytes
                               │       │                       │      │  
                               │       ▼                       │      ▼  
                               ┌───────┐    `decode_eof`       ┌──────┐  
         ┌────────────────────▶│pausing│────returns `None`────▶│paused│  
         │                     └───────┘                       └──────┘  
    read 0 bytes                   ┌──────┐                        │     
         │                         │      │                        │     
         │                         │  `decode` returns `Some`      │     
     ╔═══════╗                 ┌───────┐◀─┘                        │     
     ║reading║─read n>0 bytes─▶│framing│                           │     
     ╚═══════╝                 └───────┘◀──────read n>0 bytes──────┘     
         ▲                         │                                     
         │                         │                                     
         └─`decode` returns `None`─┘                                     

It is up to the Decoder to handle restarts (aka. transitions from paused to framing) by implementing an appropriate policy.

Full disclosure: I've only been programming rust in anger for 10 days.
This solves the issues I had, but I might be ignorant of Steam, Sink and other rust semantics and best practices.

The current behaviour of FramedRead::poll_next did not align with
the behaviour of AsyncReadExt, where read operations might only
temporarily return 0 bytes (signalling EOF). And provided no way
to recover from such an event, behaving similar to a FusedStream.

Furthermore decode_eof could be called with repeated read attempts
on a stream that already returned None, potentially providing a source
of unexpected/erroneous closing frames.

The new behaviour allows for Frame consumption to continue even after
an EOF event, iff the underlying AsyncRead has more data, as with files
and FIFOS. After decode_eof has returned None it will not be called
again until new data was read and a new EOF event has been encountered.

Fixes: tokio-rs#3252
The behaviour described in tokio-rs#3252 is now better documented.
Including an example on how to resume a framed stream.
@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-codec Module: tokio-util/codec labels Dec 14, 2020
Looks like I've fixed the merge conflict too hastily.
The correct trait is imported in the test now.
@Darksonn
Copy link
Contributor

Sorry for the delay in reviewing. We were busy with getting Tokio 1.0 out.

The state machine is somewhat difficult to understand. The name readable is not very good (I know you didn't come up with it). Is it possible to have the underlying IO resource return a read of length zero without the stream returning None?

@somethingelseentirely
Copy link
Contributor Author

somethingelseentirely commented Dec 27, 2020

Hey @Darksonn
No worries, I expected 1.0 busyness 😄.
I think the state machine becomes easier if you don't associate the is_readable flag with the underlying byte source, but the buffer. It says "is the buffer potentially readable, either because we were able to load some data into the buffer, or because we're currently emitting closing frames which are not recognised by decode but by decode_eof?".

I've updated the state machine notes to use the states instead of the flags, sorry that was my error, with the problem still cached in my brain I didn't make the proper abstractions.

I've also included an ASCII drawing of the state machine and the transitions it takes when the flags change.

To answer your question, yes that's possible. It's part of the decode_eof codepath, which is supposed to emit frames that signal the end of the stream.
However it is not possible for frames to be emitted through the decode path on a 0 read.

Merry winter solstice, thanks for checking this out in-between the years 🎄 🎁 ❤️ ✨!

tokio-util/src/codec/framed_impl.rs Outdated Show resolved Hide resolved
tokio-util/src/codec/framed_impl.rs Outdated Show resolved Hide resolved
Adds ASCII art state machine drawing to better document the workings of `framed_impl`.
Also improved the documentation text to make the `is_readable` flag more clear.
This annotates the code in `poll_next` with information on which state a code-path might be in,
 which transitions a flag change might prepare, and which transitions a loop continuation or return
might actually commit to.
This documents the implicit stopped -> stopped transition
that occurs when a read is pending.
tokio-util/src/codec/decoder.rs Outdated Show resolved Hide resolved
tokio-util/src/codec/framed.rs Outdated Show resolved Hide resolved
tokio-util/src/codec/framed.rs Outdated Show resolved Hide resolved
tokio-util/src/codec/framed_impl.rs Outdated Show resolved Hide resolved
tokio-util/src/codec/framed_impl.rs Outdated Show resolved Hide resolved
tokio-util/src/codec/framed_impl.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Can you add some tests? Then I think this is ready.

Sorry for the delay in reviews.

tokio-util/src/codec/decoder.rs Outdated Show resolved Hide resolved
tokio-util/src/codec/framed.rs Outdated Show resolved Hide resolved
@Darksonn
Copy link
Contributor

Sorry, I completely forgot about this. I will merge it when CI passes.

@Darksonn Darksonn merged commit 8ed825f into tokio-rs:master Mar 22, 2021
@somethingelseentirely
Copy link
Contributor Author

Sorry, I completely forgot about this. I will merge it when CI passes.

No need to apologize.
It's my mistake for not finding the courage to have written tests earlier.
I kinda went down a rust-testing rabit hole and procrastinated on the issue itself.

Thanks a lot for writing those tests!
I owe you one. 🙇🏼

Also thanks for your effort put into merging this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate M-codec Module: tokio-util/codec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants