Refactors Sockets closed promise handling. #237

dom96 · 2022-12-22T18:10:20Z

This is a more basic version of #205. The main change in semantics here is that the closed promise will now have any errors propagated to it (from the readable/writable stream close calls). The rest is a major refactor to reuse the existing newPromisedStream.

Test Plan

$ bazel test //src/ew/tests:sockets/sockets-tcp-test.ew-test-bin --config=asan --test_timeout 5

src/workerd/api/system-streams.c++

src/workerd/api/sockets.c++

src/workerd/api/sockets.h

src/workerd/api/sockets.c++

src/workerd/api/system-streams.c++

dom96 · 2022-12-23T18:36:20Z

Last push was a rebase.

Ready for another review

src/workerd/api/sockets.c++

kentonv · 2023-01-02T23:09:48Z

Please note my reply to a previous thread (which had been marked resolved): #237 (comment)

dom96 · 2023-01-04T10:57:18Z

@kentonv responded.

dom96 · 2023-01-09T12:39:32Z

Rebased

dom96 · 2023-01-09T16:03:43Z

Ready for review.

jasnell

LGTM but let's also have @kentonv or @harrishancock review and sign off.

src/workerd/api/system-streams.c++

dom96 · 2023-01-12T16:18:41Z

Rebased

src/workerd/api/sockets.c++

src/workerd/api/sockets.h

kentonv · 2023-01-12T19:12:54Z

src/workerd/api/sockets.h

-    //       better way?
-    auto& context = IoContext::current();
-    return context.awaitIo(kj::mv(closeFulfiller->promise));
+  jsg::MemoizedIdentity<jsg::Promise<void>>& getClosed() {


It looks like, at present, this promise is only resolved when the application calls close() explicitly -- not when the remote side disconnects. Is that correct?

If so, it seems like it's not accomplishing a whole lot to offer a way to listen for closure. The application can set that up itself and save us the trouble. So I'd remove getClosed() in that case.

If the intent is that this promise should also tell the application when the remote end disconnects the socket, then I think you should add code that listens on whenWriteDisconnected() and fulfills the promise at that point. (Or maybe it should wait on both whenWriteDisconnected() and EOF or error on the read side, and only resolve when both are true... I'm not really sure... I'd be satisfied with waiting for whenWriteDisconnected() only.)

Yep, it's the current behaviour in workerd HEAD as well. I split this PR off from #205 to not block on this and instead focus on the refactoring.

I will follow up on this but first we need to decide how we want the semantics to work exactly.

src/workerd/api/sockets.h

src/workerd/api/sockets.c++

kentonv · 2023-01-13T17:14:39Z

src/workerd/api/sockets.c++

+      resolveFulfiller(nullptr);
+      return js.resolvedPromise();
+    }, [this](jsg::Lock& js, jsg::Value err) { return errorHandler(js, kj::mv(err)); });
+  }, [this](jsg::Lock& js, jsg::Value err) { return errorHandler(js, kj::mv(err)); });


So it looks like any exceptions caught from cancel() or abort() propagate to the closed promise, but do NOT propagate to the result of close() itself.

Is that what we want? I'm not really sure when cancel() or abort() would throw, but it would seem to me that the exception should propagate out of close(). Meanwhile it's not clear to me whether the closed promise should signal errors or just signal that the socket is now closed.

Well, I'd allow the error to propagate to the closed promise but I would explicitly mark it handled using markAsHandled so that if someone isn't paying attention it's perfectly ok.

Meanwhile it's not clear to me whether the closed promise should signal errors or just signal that the socket is now closed.

It's not clear to me either. Though I trust James' opinion here.

This is another case that I think we need to discuss in more depth. I want us to do so on this issue and many others, after discussion we may end up with another decision (especially if we decide to conform to other specs) so let's not worry too much about making it perfect in this PR.

src/workerd/api/sockets.h

jasnell · 2023-01-13T20:15:02Z

@dom96 ... PR needs a rebase.

dom96 requested review from kentonv and harrishancock December 22, 2022 18:10

dom96 force-pushed the dominik/cleanup-sockets-streams branch from ca3d4fa to f79c885 Compare December 22, 2022 18:13

harrishancock reviewed Dec 22, 2022

View reviewed changes

kentonv reviewed Dec 22, 2022

View reviewed changes

src/workerd/api/system-streams.c++ Outdated Show resolved Hide resolved

dom96 force-pushed the dominik/cleanup-sockets-streams branch 3 times, most recently from a5277b5 to 171059b Compare December 23, 2022 18:36

kentonv reviewed Jan 2, 2023

View reviewed changes

src/workerd/api/sockets.c++ Outdated Show resolved Hide resolved

dom96 force-pushed the dominik/cleanup-sockets-streams branch from 171059b to fc8da39 Compare January 9, 2023 12:39

dom96 force-pushed the dominik/cleanup-sockets-streams branch from fc8da39 to 46c7aa2 Compare January 9, 2023 15:54

jasnell approved these changes Jan 10, 2023

View reviewed changes

jasnell reviewed Jan 11, 2023

View reviewed changes

src/workerd/api/system-streams.c++ Outdated Show resolved Hide resolved

dom96 force-pushed the dominik/cleanup-sockets-streams branch from 46c7aa2 to b315ad3 Compare January 12, 2023 16:18

dom96 force-pushed the dominik/cleanup-sockets-streams branch from b315ad3 to 3979484 Compare January 12, 2023 16:59