-
Notifications
You must be signed in to change notification settings - Fork 290
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 various dropped promises #1222
Conversation
Sometimes the test wants to wait on the result of expectUncached(), and sometimes it doesn't. When it doesn't, cast the result to void to suppress warnings from discarding a promise. I did try waiting on some of these promises instead of dropping them, but that causes test failures. Seems like the dropping is intentional.
This looks like a simple missing "co_await". Earlier in the method, we have "co_await ws.receive()", so I think it's okay to "co_await ws.send(...)" as well.
Instead of awaiting sequentially, I opted to drain everything concurrently for lower latency.
There's no actual bug here. DigestStreamSink::write doesn't do any IO (it's a cryptographic digest; it just computes things), but it has to return kj::Promise<void> since it implements WritableStreamSink. This just silences a compiler warning.
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.
LGTM but let's have @kentonv review also
@@ -207,7 +207,7 @@ kj::Promise<void> HibernationManagerImpl::readLoop(HibernatableWebSocket& hib) { | |||
// autoResponseTimestamp on the active websocket. | |||
(active)->setAutoResponseTimestamp(hib.autoResponseTimestamp); | |||
} | |||
ws.send((reqResp)->getResponse().asArray()); | |||
co_await ws.send((reqResp)->getResponse().asArray()); |
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.
@jqmmes what would you like to do here? Have it co_await
or continue discarding until your fix is in?
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.
It's OK to merge it as is. I can update it later, once my PR is ready.
I found these by grabbing the changes from capnproto/capnproto#1810, which makes the compiler warn about dropped promises.