refactor(gateway, http, lavalink): Switch to tokio-websockets #2239

Gelbpunkt · 2023-07-07T16:28:16Z

This switches our websocket library from tokio-tungstenite to tokio-websockets.

Why?

tokio-tungstenite is notoriously slow with dependency updates (it took them one month from updating rustls to drafting a new release), which means we are held back on updating these shared dependencies
tokio-websockets pulls in fewer dependencies: a bare repository that only depends on model, http and gateway with only rustls-webpki-roots enabled sees this dependency drop when using this PR:

    Updating git repository `https://github.com/Gelbpunkt/twilight`
    Updating crates.io index
      Adding base64 v0.21.2
    Removing block-buffer v0.10.4
    Removing byteorder v1.4.3
    Removing cpufeatures v0.2.9
    Removing crypto-common v0.1.6
    Removing data-encoding v2.4.0
    Removing digest v0.10.7
      Adding fastrand v2.0.0
    Removing form_urlencoded v1.2.0
    Removing generic-array v0.14.7
    Removing getrandom v0.2.10
    Removing idna v0.4.0
    Removing ppv-lite86 v0.2.17
    Removing rand v0.8.5
    Removing rand_chacha v0.3.1
    Removing rand_core v0.6.4
    Removing sha1 v0.10.5
    Removing thiserror v1.0.43
    Removing thiserror-impl v1.0.43
    Removing tinyvec v1.6.0
    Removing tinyvec_macros v0.1.1
    Removing tokio-tungstenite v0.19.0
      Adding tokio-websockets v0.3.3
    Removing tungstenite v0.19.0
    Removing twilight-gateway v0.15.2 (https://github.com/twilight-rs/twilight?branch=next#8c48c3c7)
      Adding twilight-gateway v0.15.2 (https://github.com/Gelbpunkt/twilight?branch=tokio-websockets#b8234991)
    Removing twilight-gateway-queue v0.15.2 (https://github.com/twilight-rs/twilight?branch=next#8c48c3c7)
      Adding twilight-gateway-queue v0.15.2 (https://github.com/Gelbpunkt/twilight?branch=tokio-websockets#b8234991)
    Removing twilight-http v0.15.2 (https://github.com/twilight-rs/twilight?branch=next#8c48c3c7)
      Adding twilight-http v0.15.2 (https://github.com/Gelbpunkt/twilight?branch=tokio-websockets#b8234991)
    Removing twilight-http-ratelimiting v0.15.1 (https://github.com/twilight-rs/twilight?branch=next#8c48c3c7)
      Adding twilight-http-ratelimiting v0.15.1 (https://github.com/Gelbpunkt/twilight?branch=tokio-websockets#b8234991)
    Removing twilight-model v0.15.2 (https://github.com/twilight-rs/twilight?branch=next#8c48c3c7)
      Adding twilight-model v0.15.2 (https://github.com/Gelbpunkt/twilight?branch=tokio-websockets#b8234991)
    Removing twilight-validate v0.15.1 (https://github.com/twilight-rs/twilight?branch=next#8c48c3c7)
      Adding twilight-validate v0.15.1 (https://github.com/Gelbpunkt/twilight?branch=tokio-websockets#b8234991)
    Removing typenum v1.16.0
    Removing unicode-bidi v0.3.13
    Removing unicode-normalization v0.1.22
    Removing url v2.4.0
    Removing utf-8 v0.7.6
    Removing version_check v0.9.4
    Removing webpki v0.22.0

tokio-websockets is actively maintained (by me). I would not mind whatsoever moving it to the twilight organization to make it more accessible to fellow twilight maintainers
I have done excessive testing of tokio-websockets against the autobahn test suite that verifies RFC compliance of websocket libraries, tokio-websockets passes all cases it implements (see the results) and implements the entire required specification which is also used by Discord
Benchmark results show massive performance gains over tokio-tungstenite under high loads
tokio-websockets supports SIMD-accelerated masking and unmasking of websocket frames on x86_64, aarch64 and ARMv7 and can also use SIMD for UTF-8 validation, while tokio-tungstenite has to rely on auto-vectorization
The entire public API of twilight remains the same with this change (PR is against next because this is still a too big change to be deployed in a patch release)
Allows us to drop our custom TlsConnector logic, since tokio-websockets has this in-built

Why not?

tokio-tungstenite is probably more well-tested, despite tokio-websockets being used in some production applications for 1-2 years now
tokio-tungstenite has corporate backing in maintainership (although that argument doesn't hold up well if we look at the frequency of their commits)

Notes

~~Currently using a git dependency on tokio-websockets because I've added a few changes for easier integration with twilight that will require a new minor release~~ I released 0.4.0 now
~~The entire twilight integration is completely untested~~ I've verified that the gateway implementation runs shards properly for a long time (with several resumes), lavalink needs testing
I've ditched rand since it is a bit big for our uses and switched to fastrand because tokio-websockets supports it and uses it by default. I could revert that for now if desired.

TODO

Test test test test and did I mention testing?
~~Need to figure how to make cargo-hack skip the --no-default-features case and instead enable the no-tls feature for that~~ Just always enable sha1_smol

laralove143 · 2023-07-07T17:11:38Z

i like this change overall, but users might expect us to use de facto libraries, how much maintenance cost would it add to offer this under a feature flag?

Gelbpunkt · 2023-07-09T17:25:28Z

i like this change overall, but users might expect us to use de facto libraries, how much maintenance cost would it add to offer this under a feature flag?

Impossible because we would have to make both websocket libraries opt-in to avoid pulling in both for those who do not want to use the other one. It wouldn't be worth the maintainance cost either and would probably require abstracting out a lot of things here in not-so-pretty-ways. In my view we should use whatever we consider superior, the concept of "de facto libraries" is flawed if the libraries are unmaintained or otherwise abandoned, slow or bloated. In my view, if we consider something to be superior, we should use it.

laralove143 · 2023-07-09T19:59:40Z

i like this change overall, but users might expect us to use de facto libraries, how much maintenance cost would it add to offer this under a feature flag?

Impossible because we would have to make both websocket libraries opt-in to avoid pulling in both for those who do not want to use the other one. It wouldn't be worth the maintainance cost either and would probably require abstracting out a lot of things here in not-so-pretty-ways. In my view we should use whatever we consider superior, the concept of "de facto libraries" is flawed if the libraries are unmaintained or otherwise abandoned, slow or bloated. In my view, if we consider something to be superior, we should use it.

i dont agree with the de facto libraries are better idea either i was just mentioning what most users would expect from twilight

whether we decide twilight isnt for these people is another discussion but unless we jump to that we should ask for user feedback imo

twilight-gateway/Cargo.toml

Signed-off-by: Jens Reidel <[email protected]>

vilgotf

I'd also like to see the "The shard is considered disconnected after having received a close frame or encountering a websocket error, but it should only reconnect after the underlying TCP connection is closed by the server (having returned Ok(None))." be replaced by:
"The should should only reconnect after the gateway closes the underlying TCP connection."

vilgotf · 2023-09-10T19:57:57Z

twilight-http/src/request/multipart.rs

    let mut boundary = [0; 15];
-    let mut rng = rand::thread_rng();

    for value in &mut boundary {
-        *value = rng.sample(Alphanumeric);
+        *value = fastrand::alphanumeric() as u8;
    }

    boundary


Suggested change

let mut boundary = [0; 15];

let mut rng = rand::thread_rng();

for value in &mut boundary {

*value = rng.sample(Alphanumeric);

*value = fastrand::alphanumeric() as u8;

}

boundary

fastrand::u128(..).to_ne_bytes()

That won't work, boundaries have to be ASCII (Unlike some other parameter values, the values of the charset parameter are NOT case sensitive. The default character set, which must be assumed in the absence of a charset parameter, is US-ASCII)

Looking further into this, I've found conflicting resources on whether the boundary should be randomized or not, and notably I've not found any such mention in an RFC. Do you happen to know? Otherwise it seems silly to randomize the boundary at all.

Boubdaries can be arbitrary, as long as they're ASCII. Ideally they shouldn't appear in the form content, but that's all really. We could get away with using a static boundary.

Let's leave this as is for now. The issue is tracked in #2278

twilight-gateway/src/shard.rs

Signed-off-by: Jens Reidel <[email protected]>

vilgotf · 2023-09-15T10:51:16Z

twilight-gateway/src/shard.rs

+                            // Abnormal closure without close frame exchange.
+                            self.disconnect(CloseInitiator::None);
+                            self.reconnect(None, 0).await?;


Actually, we do need to return something here to indicate upstream that the shard's closing. Again, let's leave this as is but remember to check back on this prior to a release. I plan on rewriting large parts of the internals of Shard (cancel safety) during which I'll fix this too.

Let's get this merged into next very soon then so we can start refactoring the shard internals

twilight-lavalink/src/node.rs

Signed-off-by: Jens Reidel <[email protected]>

Erk-

Looks good, if we run into any issues we should be able to resolve them before a release.

github-actions bot added the c-all Affects all crates or the project as a whole label Jul 7, 2023

Gelbpunkt force-pushed the tokio-websockets branch from b697981 to a596c4d Compare July 8, 2023 18:40

vilgotf reviewed Jul 16, 2023

View reviewed changes

twilight-gateway/Cargo.toml Outdated Show resolved Hide resolved

Gelbpunkt force-pushed the tokio-websockets branch 6 times, most recently from 14cf608 to 7f3ebad Compare September 10, 2023 17:23

Gelbpunkt marked this pull request as ready for review September 10, 2023 17:24

Gelbpunkt force-pushed the tokio-websockets branch 2 times, most recently from f586c8b to c5709b0 Compare September 10, 2023 17:36

refactor(gateway, lavalink): Switch to tokio-websockets

9b4b874

Signed-off-by: Jens Reidel <[email protected]>

Gelbpunkt force-pushed the tokio-websockets branch from c5709b0 to 9b4b874 Compare September 12, 2023 22:55

Adjust for different EOF behavior

3376f33

Signed-off-by: Jens Reidel <[email protected]>

vilgotf requested changes Sep 14, 2023

View reviewed changes

Apply review suggestions

ce9ab91

Signed-off-by: Jens Reidel <[email protected]>

vilgotf mentioned this pull request Sep 15, 2023

Investigate static HTTP form boundary #2278

Open

vilgotf approved these changes Sep 15, 2023

View reviewed changes

vilgotf reviewed Sep 15, 2023

View reviewed changes

JohannesIBK reviewed Sep 16, 2023

View reviewed changes

twilight-lavalink/src/node.rs Outdated Show resolved Hide resolved

Fix apostrophes and fix logic change

1747a77

Signed-off-by: Jens Reidel <[email protected]>

Erk- approved these changes Sep 20, 2023

View reviewed changes

Gelbpunkt removed w-unapproved Proposal for change has *not* been approved by @twilight-rs/core. w-needs-testing PR needs to be tested. labels Sep 20, 2023

Gelbpunkt merged commit 0914338 into twilight-rs:next Sep 20, 2023
10 checks passed

Gelbpunkt deleted the tokio-websockets branch September 20, 2023 19:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(gateway, http, lavalink): Switch to tokio-websockets #2239

refactor(gateway, http, lavalink): Switch to tokio-websockets #2239

Gelbpunkt commented Jul 7, 2023 •

edited

Loading

laralove143 commented Jul 7, 2023

Gelbpunkt commented Jul 9, 2023 •

edited

Loading

laralove143 commented Jul 9, 2023 •

edited

Loading

vilgotf left a comment

vilgotf Sep 10, 2023

Gelbpunkt Sep 14, 2023

vilgotf Sep 14, 2023

Gelbpunkt Sep 14, 2023

vilgotf Sep 15, 2023 •

edited

Loading

vilgotf Sep 15, 2023

Gelbpunkt Sep 15, 2023

Erk- left a comment

refactor(gateway, http, lavalink): Switch to tokio-websockets #2239

refactor(gateway, http, lavalink): Switch to tokio-websockets #2239

Conversation

Gelbpunkt commented Jul 7, 2023 • edited Loading

Why?

Why not?

Notes

TODO

laralove143 commented Jul 7, 2023

Gelbpunkt commented Jul 9, 2023 • edited Loading

laralove143 commented Jul 9, 2023 • edited Loading

vilgotf left a comment

Choose a reason for hiding this comment

vilgotf Sep 10, 2023

Choose a reason for hiding this comment

Gelbpunkt Sep 14, 2023

Choose a reason for hiding this comment

vilgotf Sep 14, 2023

Choose a reason for hiding this comment

Gelbpunkt Sep 14, 2023

Choose a reason for hiding this comment

vilgotf Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

vilgotf Sep 15, 2023

Choose a reason for hiding this comment

Gelbpunkt Sep 15, 2023

Choose a reason for hiding this comment

Erk- left a comment

Choose a reason for hiding this comment

Gelbpunkt commented Jul 7, 2023 •

edited

Loading

Gelbpunkt commented Jul 9, 2023 •

edited

Loading

laralove143 commented Jul 9, 2023 •

edited

Loading

vilgotf Sep 15, 2023 •

edited

Loading