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

Update stream impl for Framed to return None after Err #4166

Merged
merged 8 commits into from
Oct 26, 2021

Conversation

bIgBV
Copy link
Contributor

@bIgBV bIgBV commented Oct 12, 2021

Motivation

Solution

Introduce a new terminal state to the Framed Stream impl which is
reached whenever the underlying decoder returns an Err. Once we reach
this state, we always return None when the stream is polled.

Introduce a new terminal state to the `Framed` `Stream` impl which is
reached whenever the underlying decoder returns an `Err`. Once we reach
this state, we always return `None` when the stream is polled.
@bIgBV
Copy link
Contributor Author

bIgBV commented Oct 12, 2021

I'll be adding tests to this tomorrow.

@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-codec Module: tokio-util/codec labels Oct 12, 2021
Comment on lines 157 to 161
// Return `None` if we have encountered an error from the underlying decoder
// See: https://github.com/tokio-rs/tokio/issues/3976
if state.has_errored {
return Poll::Ready(None);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently the stream is not fused when you reach EOF normally. Maybe we should put it back into the paused state after returning None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure what that means. If we move to the paused state, that means that the errored state is no longer terminal. I thought we wanted to return None to the stream after the underlying decoder errored out?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but do we want to keep returning none if they keep trying to read from it? See #3272 for context.

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 I see what you mean now. Yeah, it makes sense to move it to the paused state after returning None.

Comment on lines 138 to 155
// | errored | <any> | <any> | true | `decode_eof` returns Err
// ┌───────────────────────────────────────┐
// `decode_eof` │ │
// returns Ok(`Some`) │ read 0 bytes │
// │ │ ┌───────┘ │ │ │
// │ ▼ │ │ ▼ │
// ┌───────────┐ `decode_eof` ┌──────┐ │
// ┌──read 0 bytes──▶│ pausing │─returns Ok(`None`)─▶│paused│───────┐ │
// │ └───────────┘ └──────┘ │ ▼
// pending read┐ │ ┌──────┐ │ ▲ │ ┌─────────┐
// │ │ │ │ │ │ │ │ │ errored │
// │ ▼ │ │ `decode` returns `Some` │ pending read └─────────┘
// │ ╔═══════╗ ┌───────────┐◀─┘ │ ▲
// └──║reading║─read n>0 bytes─▶│ framing │ │ │
// ╚═══════╝ └───────────┘◀──────read n>0 bytes┘ │
// ▲ │ │ │
// │ │ │ `decode` returns Err │
// └─`decode` returns `None`──┘ └────────────────────────────────────────────────┘
Copy link
Contributor

Choose a reason for hiding this comment

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

This picture is kinda broken. I think you need to adjust some spaces to make stuff line up.

Copy link
Contributor Author

@bIgBV bIgBV Oct 12, 2021

Choose a reason for hiding this comment

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

I was afraid this might happen. It looks fine on my machine.
image

Let me try fixing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

You might need to use a different character for the arrows.

Copy link
Contributor

Choose a reason for hiding this comment

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

The diagram is still pretty broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied the characters being used currently for the arrows. Also, it seems to render fine in the diff on my end as well
image

I wonder if I could clean it up by using plain old ascii instead of the unicode characters.

Copy link
Contributor

Choose a reason for hiding this comment

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

that might indeed be the best way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late reply, I was traveling over the weekend. I'll push a new revision tonight.

@@ -106,6 +106,7 @@ where
eof: false,
is_readable: false,
buffer: BytesMut::with_capacity(capacity),
has_errored: false
Copy link
Contributor

Choose a reason for hiding this comment

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

There are some rustfmt failures:

Diff in /home/runner/work/tokio/tokio/tokio-util/src/codec/framed.rs at line 106:
                         eof: false,
                         is_readable: false,
                         buffer: BytesMut::with_capacity(capacity),
-                        has_errored: false
+                        has_errored: false,
                     },
                     write: WriteFrame::default(),
                 },
Diff in /home/runner/work/tokio/tokio/tokio-util/src/codec/framed_read.rs at line 51:
                     eof: false,
                     is_readable: false,
                     buffer: BytesMut::with_capacity(capacity),
-                    has_errored: false
+                    has_errored: false,
                 },
             },
         }
Diff in /home/runner/work/tokio/tokio/tokio-util/src/codec/framed.rs at line 106:
                         eof: false,
                         is_readable: false,
                         buffer: BytesMut::with_capacity(capacity),
-                        has_errored: false
+                        has_errored: false,
                     },
                     write: WriteFrame::default(),
                 },
Diff in /home/runner/work/tokio/tokio/tokio-util/src/codec/framed_read.rs at line 51:
                     eof: false,
                     is_readable: false,
                     buffer: BytesMut::with_capacity(capacity),
-                    has_errored: false
+                    has_errored: false,
                 },
             },
         }

You can fix them by running

rustfmt --edition 2018 $(git ls-files '*.rs')

@bIgBV
Copy link
Contributor Author

bIgBV commented Oct 13, 2021

Something I'd like to do as an improvement would be to refactor the stream state (ReadFrame) to use types instead of bool flags for the state machine itself. Having 3 flags and 5 states is getting fairly complex.

@Darksonn
Copy link
Contributor

If you want to refactor it, that's fine with me.

@bIgBV
Copy link
Contributor Author

bIgBV commented Oct 14, 2021

I'll do the refactor as a separate PR.

Comment on lines 56 to 59
// ================= Mock =================
struct Mock {
calls: VecDeque<io::Result<Vec<u8>>>,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason you aren't using the mock in tokio-test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the pattern in the other codec tests mostly. I saw that there's a lot of duplication, but didn't know about tokio-test itself. Do you want me to move this test to use tokio-test?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be cleaner to use tokio-test.

@bIgBV
Copy link
Contributor Author

bIgBV commented Oct 23, 2021

Not entirely sure why the bsd tests are failing.

@Darksonn
Copy link
Contributor

You might need to merge in master to get #4189.

tokio-util/tests/framed_stream.rs Outdated Show resolved Hide resolved
@stephank
Copy link

stephank commented Nov 4, 2021

Please note this change has been breaking for the redis crate. See: redis-rs/redis-rs#558 and redis-rs/redis-rs#559

Basically, redis had an impl that communicated non-fatal / server errors in Decoder::Err, in addition to errors from decoding itself. I understand the new behavior may have been intended, but may also have been better left for a 0.7. Not sure if the damage is done, now, though. 😕

@Darksonn
Copy link
Contributor

Darksonn commented Nov 4, 2021

We could revert. There are other changes pending for an 0.7 anyway. Maybe you could join our discord so we can discuss?

@stephank
Copy link

stephank commented Nov 4, 2021

Well, I was able to fix it in the redis crate and it only affected internals. The maintainer made a patch release, so my issue is fixed.

I guess we could wait it out and see if anyone else shows up? 🤷

@jszwedko
Copy link

Just for posterity, we were likewise bitten by this change in Vector: vectordotdev/vector#11254 . Our Decoders also return non-fatal errors that are just logged and then the next frame is processed. We are still trying to find the best way to handle this new tokio-util behavior.

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

4 participants