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

bIgBV · 2021-10-12T04:52:37Z

Motivation

Addresses Decoder::decode() says an Err informs Framed that the stream should be terminated, but Framed doesn't terminate #3976

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 · 2021-10-12T04:52:50Z

I'll be adding tests to this tomorrow.

Darksonn · 2021-10-12T07:05:27Z

tokio-util/src/codec/framed_impl.rs

+            // 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);
+            }


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

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?

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

Ah I see what you mean now. Yeah, it makes sense to move it to the paused state after returning None.

Darksonn · 2021-10-12T07:06:34Z

tokio-util/src/codec/framed_impl.rs

+        // | 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`──┘     └────────────────────────────────────────────────┘


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

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

Let me try fixing it.

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

The diagram is still pretty broken.

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

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

that might indeed be the best way.

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

Darksonn · 2021-10-12T07:07:37Z

tokio-util/src/codec/framed.rs

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


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 · 2021-10-13T22:57:11Z

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 · 2021-10-14T07:24:55Z

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

bIgBV · 2021-10-14T16:20:44Z

I'll do the refactor as a separate PR.

Darksonn · 2021-10-19T08:15:50Z

tokio-util/tests/framed_stream.rs

+// ================= Mock =================
+struct Mock {
+    calls: VecDeque<io::Result<Vec<u8>>>,
+}


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

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?

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

bIgBV · 2021-10-23T05:41:21Z

Not entirely sure why the bsd tests are failing.

Darksonn · 2021-10-23T09:45:08Z

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

tokio-util/tests/framed_stream.rs

…kio-rs#4166)

stephank · 2021-11-04T09:54:08Z

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 · 2021-11-04T10:31:40Z

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

stephank · 2021-11-04T14:57:16Z

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 · 2022-07-27T21:23:51Z

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.

Update stream impl for Framed to return None after Err

3ae1591

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.

Darksonn added A-tokio-util Area: The tokio-util crate M-codec Module: tokio-util/codec labels Oct 12, 2021

Darksonn reviewed Oct 12, 2021

View reviewed changes

Bhargav Voleti added 2 commits October 13, 2021 14:22

Add framed stream test

b098fea

Move stream to the paused state after returning None

83f88bd

Try fixing diagram formatting

0d504bd

Give fresh diagram a try

0577753

Darksonn reviewed Oct 19, 2021

View reviewed changes

Convert test to tokio-test

73d6d3f

Merge branch 'master' into update-decoder

0154048

Darksonn approved these changes Oct 26, 2021

View reviewed changes

tokio-util/tests/framed_stream.rs Outdated Show resolved Hide resolved

Update tokio-util/tests/framed_stream.rs

d387593

Darksonn merged commit 0c68b89 into tokio-rs:master Oct 26, 2021

Darksonn mentioned this pull request Oct 26, 2021

chore: prepare tokio-util 0.6.9 #4199

Merged

bIgBV deleted the update-decoder branch October 26, 2021 16:01

oliver-giersch pushed a commit to oliver-giersch/tokio that referenced this pull request Oct 28, 2021

codec: update stream impl for Framed to return None after Err (to…

2db263e

…kio-rs#4166)

Darksonn mentioned this pull request Oct 30, 2021

Decoder::decode() says an Err informs Framed that the stream should be terminated, but Framed doesn't terminate #3976

Closed

stephank mentioned this pull request Nov 3, 2021

"broken pipe" since tokio-util 0.6.9 redis-rs/redis-rs#558

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bIgBV commented Oct 12, 2021

bIgBV commented Oct 12, 2021

Darksonn Oct 12, 2021

bIgBV Oct 12, 2021

Darksonn Oct 12, 2021

bIgBV Oct 13, 2021

Darksonn Oct 12, 2021

bIgBV Oct 12, 2021 •

edited

Darksonn Oct 12, 2021

Darksonn Oct 14, 2021

bIgBV Oct 14, 2021

Darksonn Oct 15, 2021

bIgBV Oct 18, 2021

Darksonn Oct 12, 2021

bIgBV commented Oct 13, 2021

Darksonn commented Oct 14, 2021

bIgBV commented Oct 14, 2021

Darksonn Oct 19, 2021

bIgBV Oct 19, 2021

Darksonn Oct 19, 2021

bIgBV commented Oct 23, 2021

Darksonn commented Oct 23, 2021

stephank commented Nov 4, 2021

Darksonn commented Nov 4, 2021

stephank commented Nov 4, 2021

jszwedko commented Jul 27, 2022

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

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

Conversation

bIgBV commented Oct 12, 2021

Motivation

Solution

bIgBV commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bIgBV Oct 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bIgBV commented Oct 13, 2021

Darksonn commented Oct 14, 2021

bIgBV commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bIgBV commented Oct 23, 2021

Darksonn commented Oct 23, 2021

stephank commented Nov 4, 2021

Darksonn commented Nov 4, 2021

stephank commented Nov 4, 2021

jszwedko commented Jul 27, 2022

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

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

bIgBV Oct 12, 2021 •

edited