Update sockets to use new pipelining support in capnproto. #214
Conversation
|
I don't know why that link didn't work... |
src/workerd/api/sockets.c++
Outdated
| // the Socket to throw an appropriate error. Right now in this circumstance, | ||
| // `request.connection`'s operations will throw KJ exceptions which will be exposed to the | ||
| // script as internal errors. | ||
| return jsg::alloc<Socket>(kj::mv(request.connection)); |
There was a problem hiding this comment.
I was surprised to find that the code doesn't work (stream never receives any data). Looking into this it seems we need to attach the status promise:
| return jsg::alloc<Socket>(kj::mv(request.connection)); | |
| return jsg::alloc<Socket>(request.connection.attach(kj::mv(request.status))); |
There was a problem hiding this comment.
This is weird, attaching this promise shouldn't make any difference, because request.connection already contains its own copy of the promise (two branches of the same split()). That said, this doesn't cause any real harm except extra allocations, so I'm OK with merging in this form if we cannot immediately figure out why it helps. But we need to follow up and figure it out, because it implies a bug somewhere.
There was a problem hiding this comment.
Just to confirm I double checked that I was using the right capnproto commits: my tests definitely fail without the status attached.
@jasnell when you get a chance, please have a look as from chatting with Kenton this seems to be a bug in capnproto/capnproto#1579
dom96
force-pushed
the
harris/pipelined-http-connect
branch
from
1ce7d4e to
28416eb
Compare
dom96
force-pushed
the
harris/pipelined-http-connect
branch
from
28416eb to
0a83075
Compare
|
Okay, once tests pass I'll be merging this. |
This is a simplified version of #200, containing just the diff required to be able to merge capnproto/capnproto#1579.