nodejs / undici

An HTTP/1.1 client, written from scratch for Node.js
https://nodejs.github.io/undici
MIT License
6.08k stars 529 forks source link

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

Open jackschu opened 8 months ago

jackschu commented 8 months ago

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

metcoder95 commented 8 months ago

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.

mertcanaltin commented 8 months ago

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

jackschu commented 8 months ago

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 commented 7 months ago

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 commented 7 months ago

Can you provide the full Minimum Reproducible Example?

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

mertcanaltin commented 7 months ago

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