Application-level auto-response for hibernatable web sockets #456
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
5 times, most recently
from
eadf6e3
to
74eba04
Compare
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
from
ed4234e
to
850c25d
Compare
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
2 times, most recently
from
60e4aef
to
466ddd7
Compare
jasnell
reviewed
Apr 28, 2023
jasnell
reviewed
Apr 28, 2023
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
from
b6df9f5
to
daad03d
Compare
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
2 times, most recently
from
e310b8f
to
5e0129e
Compare
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
from
5e0129e
to
7b73d31
Compare
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
from
7b73d31
to
b76993a
Compare
jqmmes
changed the title
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
from
b76993a
to
f4bdadf
Compare
jasnell
reviewed
May 11, 2023
src/workerd/api/actor-state.h
Outdated
| JSG_RESOURCE_TYPE(DurableObjectState, CompatibilityFlags::Reader flags) { | ||
| JSG_METHOD(waitUntil); | ||
| JSG_READONLY_INSTANCE_PROPERTY(id, getId); | ||
| JSG_READONLY_INSTANCE_PROPERTY(storage, getStorage); | ||
| JSG_METHOD(blockConcurrencyWhile); | ||
| JSG_METHOD(acceptWebSocket); | ||
| JSG_METHOD(getWebSockets); | ||
| JSG_METHOD(setWebSocketAutoresponse); | ||
| JSG_METHOD(unsetWebSocketAutoresponse); |
There was a problem hiding this comment.
May I suggest using a property getter/setter instead of a pair of methods?
There was a problem hiding this comment.
For the getter, do you think it should return a pair<request, response> or should there be two getters, one for request and one for response?
There was a problem hiding this comment.
If they always go together, a pair. If they are independent, separate getters/setters
jasnell
reviewed
May 11, 2023
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
from
f4bdadf
to
d65f380
Compare
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
2 times, most recently
from
f253264
to
5039bed
Compare
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
from
5039bed
to
e4cbde5
Compare
| } | ||
|
|
||
| kj::Maybe<jsg::Ref<api::WebSocketRequestResponsePair>> HibernationManagerImpl::getWebSocketAutoResponse() { | ||
| KJ_IF_MAYBE(ar, autoResponseRequestResponsePair) { |
There was a problem hiding this comment.
nit: this might become a 1 liner if you return autoResponsePair.map(...)
There was a problem hiding this comment.
That doesn't seem to work in this case :(
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
2 times, most recently
from
be9989d
to
e610f11
Compare
MellowYarker
reviewed
Jun 1, 2023
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
2 times, most recently
from
ab280ae
to
01fc6ff
Compare
MellowYarker
reviewed
Jun 1, 2023
MellowYarker
reviewed
Jun 1, 2023
MellowYarker
reviewed
Jun 1, 2023
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
2 times, most recently
from
9f8b8eb
to
6687ec0
Compare
MellowYarker
approved these changes
Jun 2, 2023
| @@ -340,6 +340,28 @@ class ActorState: public jsg::Object { | |||
| kj::Maybe<jsg::Ref<DurableObjectStorage>> persistent; | |||
| }; | |||
|
|
|||
| class WebSocketRequestResponsePair: public jsg::Object { | |||
There was a problem hiding this comment.
nit: I think this class should have a brief comment describing what it's for:
// A (`RequestMessage`, `ResponseMessage`) pair that's set on a Durable Object for
// Hibernatable WebSocket auto-response. The `RequestMessage` is automatically
// replied to with `ResponseMessage` without forcing the Durable Object to wake from hibernation.
26b1cfb
to
f65bfa0
Compare
|
Updated and rebased to pass windows tests |
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
2 times, most recently
from
af58280
to
2263ee8
Compare
This commit add a new method for hibernatable web sockets that enables a ping/pong application autoresponse, storing the last received pong timestamp. There's also a method to access the last received timestamp.
jqmmes
force-pushed
the
joaquim/ws-autoresponse
branch
from
2263ee8
to
e9e2fce
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a specific
requestis set for hiberntabale web socket, they must respond withresponse, store the timestamp and if currently hibernating, should stay hibernated.This PR adds a new
DurableObjectState::setWebSocketAutoresponse(request: string, response: string);andWebSocket::getAutoresponseTimestamp();.