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

HTTP2 Requests hang upon receiving 'Stream closed with error code NGHTTP2_INTERNAL_ERROR' #2675

Open
jackschu opened this issue Jan 30, 2024 · 6 comments
Labels
bug Something isn't working good first issue Good for newcomers H2 Pull requests or issues related to HTTP/2

Comments

@jackschu
Copy link

jackschu commented Jan 30, 2024

Bug Description

If node closes a http2stream with NGHTTP2_INTERNAL_ERROR, the await dispatch in httpNetworkFetch() in fetch/index.js will never resolve

Reproducible By

The tricky part about reproducing is understandably getting node to throw this error. Reproductions seem related to sending many large http2 post requests via the fetch api to the same endpoint while the event loop is busy.

Expected Behavior

The promise is rejected rather than left unfulfilled.

Logs & Screenshots

With NODE_DEBUG=http2 I can see code 2 (NGHTTP2_INTERNAL_ERROR) being thrown

[2024-01-30T22:18:35.084Z] [phase_0 saas_local_webhook] [err] HTTP2 14: Http2Stream 3 [Http2Session client]: closed with code 0, closed false, readable false
[2024-01-30T22:18:35.084Z] [phase_0 saas_local_webhook] [err] HTTP2 14: Http2Stream 3 [Http2Session client]: destroying stream
[2024-01-30T22:18:40.109Z] [phase_0 saas_local_webhook] [err] HTTP2 14: Http2Stream 3 [Http2Session client]: shutting down writable on _final
[2024-01-30T22:18:42.118Z] [phase_0 saas_local_webhook] [err] HTTP2 14: Http2Stream 3 [Http2Session client]: closed with code 2, closed false, readable true
[2024-01-30T22:18:42.118Z] [phase_0 saas_local_webhook] [err] HTTP2 14: Http2Stream 3 [Http2Session client]: destroying stream

If i add a call to errorRequest here in writeH2() i can see the following error and my expected behavior of rejecting the promise

[2024-01-30T22:18:42.122Z] [phase_0 saas_local_webhook] [err] [1/30/2024, 5:18:41 PM] [PLATFORM_FETCH] Error resulted from fetch for request POST 
{
  "stack": "TypeError: fetch failed\n    at Object.fetch9 (/nix/store/qwj14ylfz8y3ji41rgzssgk25i4wp749-esbuild_node/depengine_worker.js:31730:19)\n    at processTicksAndRejections (node:internal/process/task_queues:96:5)",
  "message": "fetch failed",
  "cause": {
    "stack": "Error [ERR_HTTP2_STREAM_ERROR]: Stream closed with error code NGHTTP2_INTERNAL_ERROR\n    at new NodeError (node:internal/errors:372:5)\n    at ClientHttp2Stream._destroy (node:internal/http2/core:2339:13)\n    at _destroy (node:internal/streams/destroy:109:25)\n    at ClientHttp2Stream.destroy (node:internal/streams/destroy:71:5)\n    at Http2Stream.onStreamClose (node:internal/http2/core:549:12)",
    "message": "Stream closed with error code NGHTTP2_INTERNAL_ERROR"
  }
}

Primary question

Why is errorRequest used only in the frameError handler and not the error handler? How else should this promise end up rejected?

Environment

Undici: 5.28.1
OS: nixos 5.23.11
Node: v18.18.2

@jackschu jackschu added the bug Something isn't working label Jan 30, 2024
@metcoder95
Copy link
Member

Yeah, that's a good catch and indeed a bug. Both belong to a single stream that is most likely tight to a single request/response lifecycle for fetch and undici itself.

There's no reason to reuse the same, it was most likely left over we forgot to check.

Would you like to send a PR? It might be hard, but let's see if we can add some tests.

@metcoder95 metcoder95 added the good first issue Good for newcomers label Jan 31, 2024
@mertcanaltin
Copy link
Member

I can take care of this issue if it's appropriate

@jackschu
Copy link
Author

Sure I can look at adding this patch + some test coverage, It'll likely be outside of my working hours so may be a week or so

@jackschu
Copy link
Author

jackschu commented Feb 6, 2024

I took a swing at this but am running into some trouble around ClientDestroyedError

Currently something like this would result in the promise hanging, I don't think that's correct, and should instead result in a reject

  client.on('connect', () => {
    client.close()
  })
  await client.request({
    path: '/',
    method: 'GET',
  })

but just calling errorRequest on error in client.js's stream error handler results in a ClientDestroyedError seemingly if the client is closed without the body having been consumed eg the following will throw

  await client.request({
    path: '/',
    method: 'GET',
  })
  await client.close()

Any advice around how to handle this?

@metcoder95
Copy link
Member

Can you provide the full
Minimum Reproducible Example?

Feel free to also open a PR and we can iterate from it

@mertcanaltin
Copy link
Member

I opened a pr thank you very much for your support and explanations I hope I did it right if you have any suggestions please do not hesitate @metcoder95 @jackschu

@metcoder95 metcoder95 added the H2 Pull requests or issues related to HTTP/2 label Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers H2 Pull requests or issues related to HTTP/2
Projects
None yet
Development

No branches or pull requests

3 participants