search

connectrpc / connect-es

The TypeScript implementation of Connect: Protobuf RPC that works.
https://connectrpc.com/
Apache License 2.0
1.31k stars 73 forks source link

Requests appear aborted before handler over HTTP/1.1 #1025

Open devinivy opened 6 months ago

devinivy commented 6 months ago

Describe the bug

The signal request.signal on a UniversalServerRequest is aborted as soon as the request body has been read, at least over HTTP/1.1. As a result, for most rpc handlers the request appears aborted before the handler begins (since the request has already been read). Ideally the signal would not be aborted until the underlying connection closes.

To Reproduce

Create an rpc handler using node HTTP/1.1, hit the endpoint with a POST request, and read request.signal.aborted at the top of the handler. It will appear aborted (i.e. take the value true) even though the client and server maintain a connection, and the client will be able to receive the response.

Environment (please complete the following information):

Additional context

The issue most likely goes back to this change in node v16 https://github.com/nodejs/node/issues/40775, where the 'close' event on an IncomingMessage indicates that you've read the whole message, whereas in the past it represented the resource underlying the request (i.e. would not close until the request/response flow ended, either due to actions taken by the server or client). I believe the more straightforward fix may be to use the node response object's 'close' event rather than the request object's here: https://github.com/connectrpc/connect-es/blob/fa7726b598391b8dfd0e9f14e27dcc87aca74146/packages/connect-node/src/node-universal-handler.ts#L120-L121

srikrsna-buf commented 6 months ago

Thank you for reporting and for finding the node issue! I can reproduce. Not sure if relying on the response is the solution here.

davidfiala commented 1 month ago

Accidentally I reported this on Slack without seeing this bug first.

In case it helps, I can provide additional details on my repro:

Node v22.4.1 connect rpc v1.4.0

Can confirm that when adding http2: true to the fastly options that the bug goes away, so it is definitely as as the initial reporter indicated http1. The node Http2SecureServer impl doesn't suffer.

It indeed appears to be related to the 'close' event being handled prematurely: https://github.com/connectrpc/connect-es/blob/76ee3348de3426817748ad9e135c4d5815e754f5/packages/connect-node/src/node-universal-handler.ts#L118

The abort controller .reason included:

at AbortController.abort (node:internal/abort_controller:395:18)
    at IncomingMessage.onH1Close (file:///repos/battle/.yarn/__virtual__/@connectrpc-connect-node-virtual-e05dc4290e/0/cache/@connectrpc-connect-node-npm-1.4.0-215ee814b5-8699e3cb8d.zip/node_modules/@connectrpc/connect-node/dist/esm/node-universal-handler.js:80:29)
lourd commented 2 weeks ago

I also just ran into this issue when switching a service from http2 to http. My logger includes whether or not a response was aborted. It was very confusing and concerning to see that all of our requests were now being aborted. It also broke logic in our code that uses the abort signal.

This is quite a significant bug. It effectively eliminates http1 as an option for any moderately complex Connect server.

timostamm commented 3 days ago

Thank you for reporting and for finding the node issue! I can reproduce. Not sure if relying on the response is the solution here.

@srikrsna-buf, can you still reproduce this after https://github.com/connectrpc/connect-es/pull/1206?