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

fix(schema-engine): drop buggy ws_stream_tungstenite dependency #5128

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

aqrln
Copy link
Member

@aqrln aqrln commented Jan 15, 2025

WebSocketStream in tokio-tungstenite (or its fork async-tungstenite) only implements Stream and Sink but not AsyncRead and AsyncWrite — by design, because WebSocket is a message based protocol and thinking about it in terms of byte streams usually doesn't make sense.

However, we want to tunnel a stream based protocol through WebSocket messages. It's a rather exotic use case, and normally not very useful except in web/browser based applications or when trying to bypass firewalls and do other sketchy stuff, so there's not much in the ecosystem that implements this. But we have limitations similar to those frontend applications face on the server side due the nature of the edge environement, so we don't have a choice to use TCP directly, and we used an obscure library called ws_stream_tungstenite (which is not part of tungstenite project) to implement tunneling on top of normal tungstenite websockets.

This library is not popular (34 stars on GitHub), not actively maintained (it has occasional updates but it has serious correctness issues reported in 2021 and still not fixed), and looks quite convoluted internally. We and our users constantly run into this same bug from 2021, and the best way to fix it is to just get rid of this dependency.

This PR:

  • Replaces ws_stream_tungstenite with a simple and straightforward own implementation.
  • Replaces async-tungstenite (previously required by ws_stream_tungstenite) with tokio-tungstenite (a smaller and more popular library by the authors of tungstenite itself that async-tungstenite was originally forked from).

How the implementation works:

  • On the read side, we use tokio_util::io::StreamReader to convert a Stream<Bytes> to an AsyncRead (and our newtype that also implements Stream in the middle to filter and convert WebSocket messages to byte chunks).
  • On the write side, we use a custom AsyncWrite implementation that writes each bytes chunk directly to the sink as a binary WebSocket frame. A peculiar thing about it is we poll each write to completion. Only starting the write in poll_write and relying on the caller to use poll_flush, like SinkWriter does, didn't work for me, as data never appeared on the read side after initiating a write and always returned Pending. Making each write wait for flushing and thus serializing I/O operations fixed the issue.

Fixes: https://linear.app/prisma-company/issue/ORM-504/fix-ws-stream-tungstenite-error-in-migrate

@aqrln aqrln added this to the 6.3.0 milestone Jan 15, 2025
Copy link
Contributor

github-actions bot commented Jan 15, 2025

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.106MiB 2.107MiB -906.000B
Postgres (gzip) 844.560KiB 845.262KiB -719.000B
Mysql 2.068MiB 2.067MiB 643.000B
Mysql (gzip) 830.605KiB 830.897KiB -299.000B
Sqlite 1.976MiB 1.978MiB -1.964KiB
Sqlite (gzip) 793.008KiB 793.610KiB -616.000B

Copy link

codspeed-hq bot commented Jan 15, 2025

CodSpeed Performance Report

Merging #5128 will not alter performance

Comparing push-yymymorovkmz (0d53bf8) with main (2b21c24)

Summary

✅ 11 untouched benchmarks

@aqrln aqrln force-pushed the push-yymymorovkmz branch 2 times, most recently from 80ca942 to c24fba9 Compare January 16, 2025 10:20
@aqrln aqrln force-pushed the push-yymymorovkmz branch from c24fba9 to a7afff8 Compare January 16, 2025 10:38
@aqrln aqrln force-pushed the push-yymymorovkmz branch from a7afff8 to db22e55 Compare January 16, 2025 11:11
@aqrln aqrln marked this pull request as ready for review January 16, 2025 12:43
@aqrln aqrln requested a review from a team as a code owner January 16, 2025 12:43
@aqrln aqrln requested review from jkomyno and removed request for a team January 16, 2025 12:43
Comment on lines +171 to +172
cx.waker().wake_by_ref();
Poll::Pending
Copy link
Member Author

@aqrln aqrln Jan 16, 2025

Choose a reason for hiding this comment

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

Technically we could skip an extra roundtrip back to the executor and start polling flush right away, but the code looks slightly nicer without this micro-optimization so 🤷

.map(|r| {
if let Err(e) = r {
tracing::error!("Error in PostgreSQL WebSocket connection: {e:?}");
.map(move |result| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch

cx.waker().wake_by_ref();
Poll::Pending
}
Message::Ping(_) | Message::Pong(_) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

note: tungstenite handles responding to pings automatically

Copy link
Contributor

@jkomyno jkomyno left a comment

Choose a reason for hiding this comment

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

Nice work! I added a couple of comments

@aqrln aqrln merged commit da1f85d into main Jan 17, 2025
368 checks passed
@aqrln aqrln deleted the push-yymymorovkmz branch January 17, 2025 13:17
@aqrln aqrln mentioned this pull request Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants