WebSocket Hibernation Manager #449

MellowYarker · 2023-03-15T17:36:01Z

Old PR: #339

@jqmmes, @xortive

MellowYarker · 2023-03-16T13:14:44Z

@jqmmes I got rid of the doubly linked list I was adding to KJ here:

So assuming there aren't linking issues with std::list you should be able to use my branch now.

src/workerd/api/web-socket.c++

MellowYarker · 2023-03-20T16:51:19Z

src/workerd/io/hibernation-manager.h

+  // Also note that we box the keys and values such that in the event of a hashmap resizing we don't
+  // move the underlying data (thereby keeping any references intact).
+
+  std::list<kj::Own<HibernatableWebSocket>> allWs;


Worth thinking about Harris's comment here capnproto/capnproto#1639 (comment), since HibernatableWebSocket::activeWebSocket would be in the Accepted state if it were nonnull and that has a noexcept(false) dtor.

Just happened to notice this comment, you can probably get away with invoking clear() on this list in the noexcept(false) dtor. Keep in mind that you run the risk of throwing from multiple elements of the container, which will probably terminate the program: https://en.cppreference.com/w/cpp/language/throw#Stack_unwinding.

src/workerd/api/actor-state.c++

src/workerd/api/actor-state.h

MellowYarker · 2023-03-23T18:11:59Z

src/workerd/api/actor-state.c++

+  // We need to get a HibernationManager to give the websocket to.
+  auto& a = KJ_REQUIRE_NONNULL(IoContext::current().getActor());
+  if (a.getHibernationManager() == nullptr) {
+    // TODO(now): Actually set the hibernation manager.


FYI this TODO(now) is replaced in a later commit once we actually have a HibernationManagerImpl.

src/workerd/api/hibernatable-web-socket.c++

src/workerd/io/hibernation-manager.c++

src/workerd/api/web-socket.c++

MellowYarker · 2023-05-03T20:25:57Z

610eda0
1d952cb

Talked with the team; we're going to serialize the attachment every time the setter is called so we can send a useful error to the user if it's too large. One idea that came up was introducing a getter/setter on the websocket object, since it feels weird to throw when assigning to a property (i.e., provide ws.setAttachment(obj)). @jasnell do you have any thoughts on this?

MellowYarker · 2023-05-03T21:35:24Z

Rebased on main + bumped connection limit back up because tests are passing with the lower limit.

MellowYarker · 2023-05-04T17:42:53Z

5278b5e
c866fab

Changed the api for the attachment, surprisingly turned out simpler than it was before

src/workerd/io/worker-interface.capnp

MellowYarker · 2023-05-05T19:06:00Z

@xortive I removed some of the TODO(now)s, anything left over is better left as a hint for review. Also made the ownership transfer of websocket properties more expicit by reusing the HibernationPackage when re-creating the api::WebSocket.

xortive

I think this is ready to approve after these last batch of fixes

src/workerd/api/web-socket.c++

src/workerd/api/hibernatable-web-socket.c++

src/workerd/api/hibernatable-web-socket.h

src/workerd/api/hibernatable-web-socket.c++

src/workerd/io/hibernation-manager.c++

src/workerd/api/global-scope.c++

MellowYarker · 2023-05-08T22:16:20Z

Must've lost this include in the rebase somehow? ba1b098

We want to use the existing structure of api::WebSocket for hibernatable websocket's, but the hibernation manager needs to own the kj::WebSocket. Currently, the api::WebSocket owns it, so we need to move a couple things around.

Also modifying the way we pass errors to the hibernation error event because previous assumptions (rpc related) are no longer true.

Previously we introduced the HibernationManager class, but this commit actually implements it. The HibernationManager needs to be stored somewhere that lives long, so we'll put it in the deferred proxy task.

MellowYarker · 2023-05-09T16:53:49Z

Rebased to get rid of conflicts.

MellowYarker mentioned this pull request Mar 15, 2023

[WIP] Add hibernatable websocket sub-state to api::WebSocket #339

Closed

MellowYarker force-pushed the milan/hibernation-manager branch from cfb26cf to 11b4584 Compare March 15, 2023 23:05

MellowYarker force-pushed the milan/hibernation-manager branch from 90e82fe to ded92ba Compare March 16, 2023 13:14

jasnell reviewed Mar 16, 2023

View reviewed changes

src/workerd/api/web-socket.c++ Show resolved Hide resolved

MellowYarker force-pushed the milan/hibernation-manager branch from ded92ba to f431a85 Compare March 17, 2023 17:42

MellowYarker commented Mar 20, 2023

View reviewed changes

MellowYarker force-pushed the milan/hibernation-manager branch 3 times, most recently from 9d80560 to 9b6c104 Compare March 22, 2023 21:59

MellowYarker commented Mar 23, 2023

View reviewed changes

src/workerd/api/actor-state.c++ Show resolved Hide resolved

jasnell reviewed Mar 23, 2023

View reviewed changes

src/workerd/api/actor-state.h Outdated Show resolved Hide resolved

MellowYarker force-pushed the milan/hibernation-manager branch from adc3da1 to 381b5d1 Compare March 23, 2023 18:09

MellowYarker commented Mar 23, 2023

View reviewed changes

MellowYarker force-pushed the milan/hibernation-manager branch 5 times, most recently from 6b1deb1 to 48b0ba4 Compare March 29, 2023 15:50

MellowYarker commented Mar 29, 2023

View reviewed changes

src/workerd/api/hibernatable-web-socket.c++ Show resolved Hide resolved

MellowYarker force-pushed the milan/hibernation-manager branch 4 times, most recently from 1eb4245 to c06e9bb Compare March 30, 2023 20:54

MellowYarker commented Mar 30, 2023

View reviewed changes

src/workerd/io/hibernation-manager.c++ Outdated Show resolved Hide resolved

jasnell reviewed Mar 31, 2023

View reviewed changes

src/workerd/api/web-socket.c++ Outdated Show resolved Hide resolved

MellowYarker force-pushed the milan/hibernation-manager branch 4 times, most recently from 6465e89 to c142f36 Compare April 3, 2023 15:21

MellowYarker force-pushed the milan/hibernation-manager branch from 93d96f2 to 9fd5eb1 Compare May 3, 2023 15:14

MellowYarker force-pushed the milan/hibernation-manager branch 5 times, most recently from 5c25f68 to 91070fa Compare May 3, 2023 21:35

MellowYarker force-pushed the milan/hibernation-manager branch from c866fab to b2e9ee7 Compare May 4, 2023 17:43

MellowYarker commented May 4, 2023

View reviewed changes

src/workerd/io/worker-interface.capnp Outdated Show resolved Hide resolved

MellowYarker force-pushed the milan/hibernation-manager branch from 17ad997 to b696d99 Compare May 5, 2023 19:07

xortive reviewed May 8, 2023

View reviewed changes

src/workerd/api/hibernatable-web-socket.c++ Outdated Show resolved Hide resolved

xortive reviewed May 8, 2023

View reviewed changes

src/workerd/api/hibernatable-web-socket.c++ Outdated Show resolved Hide resolved

src/workerd/io/hibernation-manager.c++ Outdated Show resolved Hide resolved

src/workerd/io/hibernation-manager.c++ Outdated Show resolved Hide resolved

MellowYarker commented May 8, 2023

View reviewed changes

src/workerd/api/global-scope.c++ Show resolved Hide resolved

MellowYarker force-pushed the milan/hibernation-manager branch 2 times, most recently from 518becd to 5c724e2 Compare May 8, 2023 21:59

MellowYarker force-pushed the milan/hibernation-manager branch 2 times, most recently from 0c50dd8 to f02b98a Compare May 8, 2023 23:05

MellowYarker added 4 commits May 9, 2023 12:45

Extract api::WebSocket's kj::WebSocket

817acbd

We want to use the existing structure of api::WebSocket for hibernatable websocket's, but the hibernation manager needs to own the kj::WebSocket. Currently, the api::WebSocket owns it, so we need to move a couple things around.

Add websocket hibernation API skeleton

a9f8614

Include wasClean in hibernatable close event.

395350c

Also modifying the way we pass errors to the hibernation error event because previous assumptions (rpc related) are no longer true.

Implement and use HibernationManagerImpl

40b7de8

Previously we introduced the HibernationManager class, but this commit actually implements it. The HibernationManager needs to be stored somewhere that lives long, so we'll put it in the deferred proxy task.

MellowYarker force-pushed the milan/hibernation-manager branch from f02b98a to 40b7de8 Compare May 9, 2023 16:53

xortive approved these changes May 9, 2023

View reviewed changes

MellowYarker merged commit 811f584 into main May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebSocket Hibernation Manager #449

WebSocket Hibernation Manager #449

MellowYarker commented Mar 15, 2023

MellowYarker commented Mar 16, 2023

MellowYarker Mar 20, 2023

bcaimano Apr 20, 2023

MellowYarker Mar 23, 2023

MellowYarker commented May 3, 2023

MellowYarker commented May 3, 2023

MellowYarker commented May 4, 2023

MellowYarker commented May 5, 2023

xortive left a comment

MellowYarker commented May 8, 2023

MellowYarker commented May 9, 2023

WebSocket Hibernation Manager #449

WebSocket Hibernation Manager #449

Conversation

MellowYarker commented Mar 15, 2023

MellowYarker commented Mar 16, 2023

MellowYarker Mar 20, 2023

Choose a reason for hiding this comment

bcaimano Apr 20, 2023

Choose a reason for hiding this comment

MellowYarker Mar 23, 2023

Choose a reason for hiding this comment

MellowYarker commented May 3, 2023

MellowYarker commented May 3, 2023

MellowYarker commented May 4, 2023

MellowYarker commented May 5, 2023

xortive left a comment

Choose a reason for hiding this comment

MellowYarker commented May 8, 2023

MellowYarker commented May 9, 2023