Skip to content

http2: initialize keepAliveTimeout and keepAliveTimeoutBuffer when allowHTTP1 is true#61713

Open
amyssnippet wants to merge 1 commit intonodejs:mainfrom
amyssnippet:fix/59783
Open

http2: initialize keepAliveTimeout and keepAliveTimeoutBuffer when allowHTTP1 is true#61713
amyssnippet wants to merge 1 commit intonodejs:mainfrom
amyssnippet:fix/59783

Conversation

@amyssnippet
Copy link
Contributor

This PR fixes an issue where keepAliveTimeout and related timeout properties were undefined on Http2SecureServer instances, even when allowHTTP1: true was set.

When falling back to HTTP/1.1, the server should respect standard HTTP timeout behaviors. This change ensures these properties are initialized with their default values (matching http.Server and https.Server), preventing inconsistent behavior during HTTP/1.1 fallback.

Fixes: #59783

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Feb 7, 2026
@codecov
Copy link

codecov bot commented Feb 7, 2026

Codecov Report

❌ Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.74%. Comparing base (3ab9dd8) to head (0efefc1).

Files with missing lines Patch % Lines
lib/internal/http2/core.js 73.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61713      +/-   ##
==========================================
+ Coverage   89.73%   89.74%   +0.01%     
==========================================
  Files         675      675              
  Lines      204502   204517      +15     
  Branches    39304    39302       -2     
==========================================
+ Hits       183502   183551      +49     
+ Misses      13283    13252      -31     
+ Partials     7717     7714       -3     
Files with missing lines Coverage Δ
lib/internal/http2/core.js 95.13% <73.33%> (-0.09%) ⬇️

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@amyssnippet
Copy link
Contributor Author

@ronag the PR is ready for review.
the failed macos test is flaky unrelated to my change, my changes are isolated to lib/internal/http2/core.js and test/parallel/test-http2-https-fallback-http-server-options.js, and the error comes out to be debugger related tests which is not introduced by this change.

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 7, 2026
@amyssnippet
Copy link
Contributor Author

@ronag kindly rerun the failing check and also run the internal jenkins ci so that changes are verified to merge

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 7, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 7, 2026
@nodejs-github-bot
Copy link
Collaborator

@amyssnippet
Copy link
Contributor Author

@ronag is everything good?? all checks successfull

kindly review, if any changes then let me know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

http2: http1 fallback missing keepAlive?

3 participants