Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Requests appear aborted before handler over HTTP/1.1 #1025

Closed
devinivy opened this issue Feb 20, 2024 · 6 comments · Fixed by #1218
Closed

Requests appear aborted before handler over HTTP/1.1 #1025

devinivy opened this issue Feb 20, 2024 · 6 comments · Fixed by #1218
Labels
bug Something isn't working

Comments

@devinivy
Copy link

devinivy commented Feb 20, 2024

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):

  • @connectrpc/connect-node version: on latest 1.3.0, likely going back further as well.
  • Node.js version: v16+

Additional context

The issue most likely goes back to this change in node v16 nodejs/node#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:

nodeRequest.once("error", onH1Error);
nodeRequest.once("close", onH1Close);

@devinivy devinivy added the bug Something isn't working label Feb 20, 2024
@srikrsna-buf
Copy link
Member

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
Copy link

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:

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
Copy link

lourd commented Aug 20, 2024

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
Copy link
Member

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 #1206?

@srikrsna-buf
Copy link
Member

I still can, the fix in #1181 seems to work, but I am trying to see if we can do this in a backward compatible way.

@timostamm
Copy link
Member

I still can, the fix in #1181 seems to work,

Thanks 😞

but I am trying to see if we can do this in a backward compatible way.

The function has a comment basically marking it as private, but I agree it would be good to not break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
5 participants