Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: Fixed the issue that there is no running request when http2 goaway #3875

Merged
merged 1 commit into from
Nov 24, 2024

Conversation

ShenHongFei
Copy link
Contributor

This relates to...

onHttp2SessionGoAway throws an error

TypeError: Cannot read properties of undefined (reading 'onError')
    at Object.errorRequest (node:internal/deps/undici/undici:3935:13)
    at ClientHttp2Session.onHttp2SessionGoAway (node:internal/deps/undici/undici:5969:8)
    at ClientHttp2Session.emit (node:events:513:28)
    at ClientHttp2Session.emit (node:domain:489:12)
    at Http2Session.onGoawayData (node:internal/http2/core:683:11)
    at Http2Session.callbackTrampoline (node:internal/async_hooks:130:17)

The reason is const request = client[kQueue][client[kRunningIdx]] where request is empty.
Then I tried adding some console.log

function onHttp2SessionGoAway (errorCode) {
  ...

  // Fail head of pipeline.
  console.log('clinet[kRunningIdx]:', clinet[kRunningIdx])  // clinet[kRunningIdx]: 2
  console.log('clinet[kPendingIdx]:', clinet[kPendingIdx])  // clinet[kPendingIdx]: 2
  console.log('clinet[kQueue].length:', clinet[kQueue].length)  // clinet[kQueue].length: 2
  console.log('clinet[kQueue]:', clinet[kQueue])  // clinet[kQueue]: [null, null]
  
  const request = client[kQueue][client[kRunningIdx]]
  client[kQueue][client[kRunningIdx]++] = null
  util.errorRequest(client, request, err)  // throws "Cannot read properties of undefined"
  client[kPendingIdx] = client[kRunningIdx]

  assert(client[kRunning] === 0)

  ...
}

Rationale

fix this issue

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

@metcoder95 metcoder95 changed the title Fixed the issue that there is no running request when http2 goaway fix: Fixed the issue that there is no running request when http2 goaway Nov 24, 2024
@metcoder95
Copy link
Member

Can you add a test for it?

@ShenHongFei
Copy link
Contributor Author

I don't know how to construct a server to reproduce the http2 goaway situation here, and it's not easy to write a test case

@metcoder95 metcoder95 merged commit 9b8abb8 into nodejs:main Nov 24, 2024
36 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 24, 2024
@ShenHongFei ShenHongFei deleted the fix-http2-goaway branch November 24, 2024 13:02
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants