-
Notifications
You must be signed in to change notification settings - Fork 250
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
Conversation
WASM Query Engine file Size
|
CodSpeed Performance ReportMerging #5128 will not alter performanceComparing Summary
|
80ca942
to
c24fba9
Compare
c24fba9
to
a7afff8
Compare
a7afff8
to
db22e55
Compare
cx.waker().wake_by_ref(); | ||
Poll::Pending |
There was a problem hiding this comment.
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| { |
There was a problem hiding this comment.
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(_) => { |
There was a problem hiding this comment.
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
There was a problem hiding this 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
WebSocketStream
intokio-tungstenite
(or its forkasync-tungstenite
) only implementsStream
andSink
but notAsyncRead
andAsyncWrite
— 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 oftungstenite
project) to implement tunneling on top of normaltungstenite
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:
ws_stream_tungstenite
with a simple and straightforward own implementation.async-tungstenite
(previously required byws_stream_tungstenite
) withtokio-tungstenite
(a smaller and more popular library by the authors oftungstenite
itself thatasync-tungstenite
was originally forked from).How the implementation works:
tokio_util::io::StreamReader
to convert aStream<Bytes>
to anAsyncRead
(and our newtype that also implementsStream
in the middle to filter and convert WebSocket messages to byte chunks).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 inpoll_write
and relying on the caller to usepoll_flush
, likeSinkWriter
does, didn't work for me, as data never appeared on the read side after initiating a write and always returnedPending
. 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