-
Notifications
You must be signed in to change notification settings - Fork 252
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 hibernatable websocket race between auto-response and regular messages #1130
Conversation
da9a98f
to
275fca0
Compare
0b11e54
to
03e0f58
Compare
d9a307d
to
fad8ffe
Compare
src/workerd/api/web-socket.c++
Outdated
@@ -531,7 +531,10 @@ void WebSocket::send(jsg::Lock& js, kj::OneOf<kj::Array<byte>, kj::String> messa | |||
|
|||
KJ_UNREACHABLE; | |||
}(); | |||
outgoingMessages->insert(GatedMessage{kj::mv(maybeOutputLock), kj::mv(msg)}); | |||
|
|||
auto n = autoResponseStatus.pendingAutoResponseQueue.size() - autoResponseStatus.queuedAutoResponses; |
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.
Can you explain the difference between GatedMessage::pendingAutoResponses
, and AutoResponse::queuedAutoResponses
?
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.
GatedMessage::pendingAutoResponses
stores the number of AutoResponse::queuedAutoResponses
that will be pumped before sending the current GatedMessage
, guaranteeing order. AutoResponse::queuedAutoResponses
stores the total number of already queued pendingAutoResponses
, so we can check how many are yet to be added during each GateMessage
instantiation.
7352e8c
to
6c636ab
Compare
b9ffe75
to
8917df7
Compare
@jasnell can I get another pair of eyes reviewing this PR if you have time? |
8917df7
to
7778033
Compare
// Keep track of current hibernatable websockets auto-response status to avoid racing | ||
// between regular websocket messages, and auto-responses. | ||
struct AutoResponse { | ||
kj::Promise<void> ongoingAutoResponse = kj::READY_NOW; |
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.
Ahhhh, so I think what I was expecting was that outgoingAutoResponse
would be a forked promise ala:
kj::Promise<void> ongoingAutoResponse = kj::Proimse<void>(kj::READY_NOW).fork();
I think what you have here works, it's just a touch unusual! I'm guessing it was hard to phrase sendAutoResponse()
in that context.
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.
Thanks Joaquim!
This PR fixes possible racing issues between regular websocket messages and auto-responses.
7778033
to
1754538
Compare
This PR fixes possible
ws.send
races between auto-response messages and regular web sockets.