breaking: update tokio-tungstenite to 0.26 and update examples #3078

adryzz · 2024-12-15T22:06:41Z

Motivation

tokio-tungstenite had a new (breaking) release, 0.25/0.26 that finally allows sending shared data over websockets, and it would be cool if this new breaking change could get in axum 0.8.0 given it's a new major release

Solution

the websocket extract has been changed, and it now exposes Bytes and Utf8Bytes (UTF-8 wrapper around Bytes)

yanns · 2024-12-16T08:02:29Z

The CI checks are failing. Can you check?

adryzz · 2024-12-16T08:26:32Z

given that we have our own downstream Message type that is 1:1 with tungstenite's, should we have our own downstream Payload/Utf8Payload types too?

also, will clean up the commits in a bit

jplatte · 2024-12-16T10:21:34Z

Yes, we don't want to have tungstenite as a public dep. However I'm a bit undecided about what to expose exactly - I'll comment later after work.

Also cc @SabrinaJewson

SabrinaJewson · 2024-12-16T18:41:05Z

So, my intial observation is that Tungstenite’s Payload type is very weirdly-designed. It has three separate variants for BytesMut, Bytes and Vec<u8>. But the thing is:

the BytesMut variant seems pointless, since one can convert Bytes ↔ BytesMut with zero cost;
the only benefit of having the Vec<u8> variant is to avoid a small heap allocation if one has a Vec<u8> with excess capacity.

To me, neither of these advantage seem worth it to add a whole new Payload type to the public API instead of using Bytes directly (and I do question why tungstenite made this strange design choice). For the UTF-8 case, it seems like this should come from a utility crate, but in the absence of that making our own UTF-8 wrapper of Bytes isn’t too bad…? related: tokio-rs/bytes#697

jplatte · 2024-12-16T20:56:22Z

Those were pretty much my thoughts looking at the tungstenite change too 😅

So let's switch to Bytes then? And a Utf8Bytes type in addition? Pretty sure we even have one in the codebase already, used for path deserialization IIRC. Just not public at the moment.

alexheretic · 2024-12-17T10:20:00Z

The payload variants do have reasons to exist. For example, removing Payload::Vec variant impacts benched write performance by 10-30%. See snapview/tungstenite-rs#465 & snapview/tungstenite-rs#462

axum/src/extract/ws.rs

examples/chat/src/main.rs

alexheretic · 2024-12-17T12:30:23Z

examples/websockets-http2/src/main.rs

@@ -75,7 +75,7 @@ async fn ws_handler(
                res = ws.recv() => {
                    match res {
                        Some(Ok(ws::Message::Text(s))) => {
-                            let _ = sender.send(s);
+                            let _ = sender.send(s.to_string());


This use case could show the shared bytes reuse.

This could probably be left as a follow up task, if so maybe just add a TODO here?

An issue usually works better than a fixme comment for stuff like this. Feel free to open one.

examples/websockets/src/client.rs

examples/websockets/src/main.rs

SabrinaJewson · 2024-12-17T13:01:12Z

For example, removing Payload::Vec variant impacts benched write performance by 10-30%.

Do you know why that’s the case? Is it really just the memcpy due to having excess capacity in the Vec? Is it the cost of virtualization?

Looking at the implementation of Bytes, it should be possible to avoid the memcpy. Currently, Bytes (in its Vec representation) stores the base ptr twice, which is unnecessary; if it instead used that field to store capacity, there would be no need for shrinking the allocation. Maybe we could look into making this change in Bytes upstream, thus allowing the storage of Vec<u8> inside Bytes to be truly zero-cost?

The reason I’m somewhat against a Payload enum is that it strikes me as a code smell: Bytes is already intended to be the type representing “an enum over all the 'static thread-safe bytes containers”. So the addition of Payload means we have two layers of bytes-type-that-can-hold-any-container. If one is to go ahead with the design, I think it should be made very clear in the documentation what the exact limitations of Bytes are that it can’t subsume this use-case.

alexheretic · 2024-12-17T15:16:51Z

I do agree, I think payload started as Bytes. It is just the performance regressions driving the enum. I'm (re)investigating if those perf issues can be resolved: snapview/tungstenite-rs#473

alexheretic · 2024-12-17T16:04:23Z

It looks like the perf issues can be resolved. If possible please review that upstream PR and suggest changes so we can get the next breaking tungstenite version as right as possible.

adryzz · 2024-12-17T19:24:10Z

alright, will update this PR as soon as i get home with the newly released version.

what to do about Utf8Bytes? i can't seem to find a similiar type in the codebase already

jplatte · 2024-12-17T19:30:40Z

I just checked, looks like I was wrong. I think we can just newtype tungstenite's type then.

…g to Utf8Bytes

axum/src/extract/ws.rs

examples/chat/src/main.rs

examples/testing-websockets/Cargo.toml

axum/src/extract/ws.rs

Co-Authored-By: Alex Butler <[email protected]>

jplatte

There's a few things I'd like to look at and possibly improve before releasing, but this is definitely a big step in the right direction so I'll merge 👍

jplatte · 2024-12-18T20:13:29Z

axum/src/extract/ws.rs

            Self::Binary(ref data) | Self::Ping(ref data) | Self::Pong(ref data) => {
                Ok(std::str::from_utf8(data).map_err(Error::new)?)
            }
            Self::Close(None) => Ok(""),
            Self::Close(Some(ref frame)) => Ok(&frame.reason),
        }
    }
+
+    /// Create a new text WebSocket message from a stringable.


Not sure how to word this, but "stringable" is not a word in my vocabulary :D

jplatte · 2024-12-18T20:13:59Z

axum/src/extract/ws.rs

+impl<T> PartialEq<T> for Utf8Bytes
+where
+    for<'a> &'a str: PartialEq<T>,


Note to self: revisit.

adryzz force-pushed the tokio-tungstenite-25 branch 2 times, most recently from 42bdf98 to 23f96a3 Compare December 15, 2024 22:13

alexheretic reviewed Dec 17, 2024

View reviewed changes

axum/src/extract/ws.rs Outdated Show resolved Hide resolved

alexheretic reviewed Dec 17, 2024

View reviewed changes

axum/src/extract/ws.rs Outdated Show resolved Hide resolved

alexheretic reviewed Dec 17, 2024

View reviewed changes

examples/chat/src/main.rs Outdated Show resolved Hide resolved

alexheretic reviewed Dec 17, 2024

View reviewed changes

examples/websockets/src/client.rs Outdated Show resolved Hide resolved

alexheretic reviewed Dec 17, 2024

View reviewed changes

examples/websockets/src/main.rs Outdated Show resolved Hide resolved

alexheretic mentioned this pull request Dec 17, 2024

Rework as Bytes-only payload snapview/tungstenite-rs#473

Merged

jplatte added this to the 0.8 milestone Dec 17, 2024

adryzz changed the title ~~breaking: update tokio-tungstenite to 0.25 and update examples~~ breaking: update tokio-tungstenite to 0.26 and update examples Dec 18, 2024

adryzz added 2 commits December 18, 2024 10:57

breaking: update tokio-tungstenite to 0.25 and update examples

67fa445

breaking: update to tokio-tungstenite 0.26 and update examples

67c2ffc

adryzz force-pushed the tokio-tungstenite-25 branch from cf67e9f to 67c2ffc Compare December 18, 2024 09:59

adryzz added 4 commits December 18, 2024 11:05

websockets: remove redundant into() conversion

a55436d

breaking: change websocket message into_text() return type from Strin…

3ceb57a

…g to Utf8Bytes

examples/chat: prefer Utf8Bytes::from_static() instead of into()

0d87d8d

run rustfmt

877ce9f

alexheretic reviewed Dec 18, 2024

View reviewed changes

axum/src/extract/ws.rs Outdated Show resolved Hide resolved

alexheretic reviewed Dec 18, 2024

View reviewed changes

axum/src/extract/ws.rs Outdated Show resolved Hide resolved

alexheretic reviewed Dec 18, 2024

View reviewed changes

axum/src/extract/ws.rs Outdated Show resolved Hide resolved

alexheretic reviewed Dec 18, 2024

View reviewed changes

examples/chat/src/main.rs Outdated Show resolved Hide resolved

alexheretic reviewed Dec 18, 2024

View reviewed changes

examples/testing-websockets/Cargo.toml Outdated Show resolved Hide resolved

jplatte reviewed Dec 18, 2024

View reviewed changes

axum/src/extract/ws.rs Outdated Show resolved Hide resolved

apply suggestions

15d50aa

Co-Authored-By: Alex Butler <[email protected]>

jplock mentioned this pull request Dec 18, 2024

Update tungstenite to 0.26 #3082

Closed

1 task

jplatte approved these changes Dec 18, 2024

View reviewed changes

jplatte merged commit 96e071c into tokio-rs:main Dec 18, 2024
18 checks passed

jplatte mentioned this pull request Dec 18, 2024

Add changelog entry for tungstenite upgrade #3083

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

breaking: update tokio-tungstenite to 0.26 and update examples #3078

breaking: update tokio-tungstenite to 0.26 and update examples #3078

adryzz commented Dec 15, 2024 •

edited

Loading

yanns commented Dec 16, 2024

adryzz commented Dec 16, 2024 •

edited

Loading

jplatte commented Dec 16, 2024

SabrinaJewson commented Dec 16, 2024

jplatte commented Dec 16, 2024

alexheretic commented Dec 17, 2024 •

edited

Loading

alexheretic Dec 17, 2024 •

edited

Loading

alexheretic Dec 18, 2024

jplatte Dec 18, 2024

SabrinaJewson commented Dec 17, 2024

alexheretic commented Dec 17, 2024 •

edited

Loading

alexheretic commented Dec 17, 2024

adryzz commented Dec 17, 2024

jplatte commented Dec 17, 2024

jplatte left a comment

jplatte Dec 18, 2024

jplatte Dec 18, 2024

breaking: update tokio-tungstenite to 0.26 and update examples #3078

breaking: update tokio-tungstenite to 0.26 and update examples #3078

Conversation

adryzz commented Dec 15, 2024 • edited Loading

Motivation

Solution

yanns commented Dec 16, 2024

adryzz commented Dec 16, 2024 • edited Loading

jplatte commented Dec 16, 2024

SabrinaJewson commented Dec 16, 2024

jplatte commented Dec 16, 2024

alexheretic commented Dec 17, 2024 • edited Loading

alexheretic Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

alexheretic Dec 18, 2024

Choose a reason for hiding this comment

jplatte Dec 18, 2024

Choose a reason for hiding this comment

SabrinaJewson commented Dec 17, 2024

alexheretic commented Dec 17, 2024 • edited Loading

alexheretic commented Dec 17, 2024

adryzz commented Dec 17, 2024

jplatte commented Dec 17, 2024

jplatte left a comment

Choose a reason for hiding this comment

jplatte Dec 18, 2024

Choose a reason for hiding this comment

jplatte Dec 18, 2024

Choose a reason for hiding this comment

adryzz commented Dec 15, 2024 •

edited

Loading

adryzz commented Dec 16, 2024 •

edited

Loading

alexheretic commented Dec 17, 2024 •

edited

Loading

alexheretic Dec 17, 2024 •

edited

Loading

alexheretic commented Dec 17, 2024 •

edited

Loading