Add a new overload of Socket::handleProxyStatus for internal use #975
Conversation
src/workerd/api/sockets.c++
Outdated
| void Socket::handleProxyStatus(jsg::Lock& js, kj::Promise<kj::Maybe<kj::Exception>> connectResult) { | ||
| // It's kind of weird to take a promise that resolves to a Maybe<Exception> but we can't just use | ||
| // a Promise<void> and put our logic in the error handler because awaitIo's errorFunc isn't passed | ||
| // the jsg::Lock, which we need to have in the error case. |
There was a problem hiding this comment.
Note for later: should we update awaitIo so that it does pass the jsg::Lock?
There was a problem hiding this comment.
we should also update it to be awaitable
There was a problem hiding this comment.
Yeah this sounds like a bug in awaitIo that we should just fix instead of work around.
There was a problem hiding this comment.
It's a little bit more than I had mentioned previously -- currently the only code path that passes a jsg::Lock is if your awaited promise resolves to a non-void result and if you're on the success callback. Even if I added a jsg::Lock to the error callback that wouldn't help in this particular case since I had been trying to use a Promise<void>, and as of today both the success and error callbacks aren't given a jsg::Lock in that case.
I'll leave it as a cleanup for now rather than boiling the ocean on all the awaitIo overloads.
src/workerd/api/sockets.c++
Outdated
| auto result = context.awaitIo(js, kj::mv(connectResult), | ||
| [this, self = JSG_THIS](jsg::Lock& js, kj::Maybe<kj::Exception> result) -> void { | ||
| KJ_IF_MAYBE(e, result) { | ||
| auto exc = kj::Exception(kj::Exception::Type::FAILED, __FILE__, __LINE__, |
There was a problem hiding this comment.
Non-blocking nit: use KJ_EXCEPTION or JSG_KJ_EXCEPTION here?
src/workerd/api/sockets.c++
Outdated
| auto exc = kj::Exception(kj::Exception::Type::FAILED, __FILE__, __LINE__, | ||
| kj::str(JSG_EXCEPTION(Error), "connection attempt failed")); | ||
| resolveFulfiller(js, exc); | ||
| readable->getController().cancel(js, nullptr).markAsHandled(js); | ||
| writable->getController().abort(js, nullptr).markAsHandled(js); |
There was a problem hiding this comment.
Nit: refactor into a function since the other handleProxyStatus has the same code
a-robinson
force-pushed
the
arobinson/handle-proxy-status
branch
from
936c7a2
to
be54eaa
Compare
|
The fixup commit is here (posting since github doesn't appear to show it anymore): 936c7a2 |
So that I can handle initial connection errors on connections that were established without using HTTP connect.
a-robinson
force-pushed
the
arobinson/handle-proxy-status
branch
from
be54eaa
to
de653b5
Compare
So that I can handle initial connection errors on connections that were established without using HTTP connect.