http2: introduce http1Options and use storeHTTPOptions and initialize keepAliveTimeout and keepAliveTimeoutBuffer when allowHTTP1 is true

[amyssnippet]

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]

Review requested:

• @nodejs/http2
• @nodejs/net

[codecov]

Codecov Report

❌ Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.74%. Comparing base (04946a7) to head (0216ca9).
⚠️ Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	68.42%	6 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #61713   +/-   ##
=======================================
  Coverage   89.73%   89.74%           
=======================================
  Files         675      675           
  Lines      204648   204657    +9     
  Branches    39330    39325    -5     
=======================================
+ Hits       183651   183676   +25     
+ Misses      13282    13260   -22     
- Partials     7715     7721    +6     

Files with missing lines	Coverage Δ
lib/internal/http2/core.js	95.19% <68.42%> (-0.15%)	⬇️

... and 51 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]

@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.

[amyssnippet]

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/71226/

[amyssnippet]

@ronag is everything good?? all checks successfull

kindly review, if any changes then let me know

[amyssnippet]

@ronag kindly run the internal jenkins ci once again

[pimterry]

I won't actively block this, but I think we should investigate more long-term general solutions to the problem, and it does look achievable to do so.

Would be interested in more opinions, especially if there's challenging edge cases I've missed.

[pimterry]

ronag kindly run the internal jenkins ci once again

@amyssnippet No need to individually chase people like this unless there's clearly been no progress for a week or more, please have a little patience with us! There's a minimum time before any code can be merged anyway, to leave time for people to look at it. Because the PR is marked 'author ready' everybody knows that it's already in a fairly good state, so CI will be rerun if it looks flaky, and there will be progress soon one way or the other 😃

In the meantime, if you have thoughts on my comment above that you want to share, or if you have time and energy to take a look & test whether that alternative approach works OK or causes any issues, please do.

[Qard]

Agreed with other comments that we should think on a way to pass through options more broadly rather than handling each individual case separately. But otherwise, LGTM.

[amyssnippet]

ronag kindly run the internal jenkins ci once again

@amyssnippet No need to individually chase people like this unless there's clearly been no progress for a week or more, please have a little patience with us! There's a minimum time before any code can be merged anyway, to leave time for people to look at it. Because the PR is marked 'author ready' everybody knows that it's already in a fairly good state, so CI will be rerun if it looks flaky, and there will be progress soon one way or the other 😃

In the meantime, if you have thoughts on my comment above that you want to share, or if you have time and energy to take a look & test whether that alternative approach works OK or causes any issues, please do.

i refractored it, now i made sure that there is no duplicate logic here and now it ensures we inherit all standard HTTP/1 option behaviors automatically. Thanks for the pointer!

[amyssnippet]

@pimterry Thanks for the detailed roadmap! I've implemented the changes

[Qard]

The deprecation of Http1IncomingMessage and Http1ServerResponse here is not strictly necessary. Not sure how I feel about creating that churn, even if just as a docs deprecation, when the interface that creates with http1Options is not really meaningfully different. 🤔

I won't block on it, but I feel like we may want to rethink how we deal with that option propagation...possibly we should revert that change and defer decision-making on that to another PR? Not of a strong opinion on this, but just feels a bit unideal to me.

[amyssnippet]

The deprecation of Http1IncomingMessage and Http1ServerResponse here is not strictly necessary. Not sure how I feel about creating that churn, even if just as a docs deprecation, when the interface that creates with http1Options is not really meaningfully different. 🤔

I won't block on it, but I feel like we may want to rethink how we deal with that option propagation...possibly we should revert that change and defer decision-making on that to another PR? Not of a strong opinion on this, but just feels a bit unideal to me.

as this issue is only for adding the keepAliveTimeout (and other HTTP/1 options) available on HTTP/2 servers with allowHTTP1 and that's the issue being solved. Deprecating Http1IncomingMessage/Http1ServerResponse is a separate API design decision that can be discussed independently.

i am reverting the deprecation parts

what are your thoughts @pimterry . i think we should keep http1Options documented and working i already made the api for new options and keep backward compat for the old options

[pimterry]

The deprecation of Http1IncomingMessage and Http1ServerResponse here is not strictly necessary. Not sure how I feel about creating that churn, even if just as a docs deprecation, when the interface that creates with http1Options is not really meaningfully different. 🤔

I won't block on it, but I feel like we may want to rethink how we deal with that option propagation...possibly we should revert that change and defer decision-making on that to another PR? Not of a strong opinion on this, but just feels a bit unideal to me.

I agree it's not strictly necessary, but it is tidier imo. The whole situation is a little messy really.

I do think http1Options is valuable: it tidily segregates the options, lets us solve the broader issue completely, avoids any risk of HTTP/1 vs HTTP/2 option conflicts later, and gives more control with a clearer API.

Given that, it's (almost) unavoidable that we end up with two actively supported ways to do the same thing for the IncomingMessage/ServerResponse fields and imo it's worthwhile to deprecate to avoid that. Not a strong position though, so if people really prefer to avoid the deprecation then I'm open to just supporting both options for this: the status quo + http1Options as well.

Right now, for me the current code represents my preference I think: we reorganize to a better solution, and we gently deprecate to encourage new code onto the new API, but without any warnings or anything for existing code (probably ever - fully agree it's not critical and we can just support both indefinitely, I'd just rather clearly pave the path).

[pimterry]

Ah ok, understood. Yes, they are very weird.

Open to exploring changes there, but yes let's leave it until later - the main change to fix that would be on the HTTP/1 side anyway just mirrored over here, so I don't think this changes makes that significantly more difficult.

[Qard]

Yep, I'm fine with this as-is, just we should think about a follow-up to consider better approaches to that options object.

[amyssnippet]

@pimterry i suggest this as squash and merge commit msg

http2: fixed http1 fallback timeouts, introduced http1Options and deprecated top-level fallback options

[pimterry]

Thanks for squashing that @amyssnippet. That commit message seems reasonable to me for the changelog.

It looks like another deprecation has been merged though, so this PR now has a conflict so can't be merged - you'll need to rebase on main, fix the conflict, and then force-push again update the PR.

Once that's done, I'm happy to approve and then we can kick of the CI jobs to run all the tests. We'll then need to wait for a 2nd approval (or for the PR to be open for 7 days, so there's time for any objections) before we can merge.

[amyssnippet]

@pimterry i have resolved the merge conflict, every check here is successful, so lets wait until someone else reviews and gives their comments. Thanks!

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/71310/

[amyssnippet]

Before actually landing this, we'll wait a day or two in case there are any more thoughts on the deprecation position here as well, more opinions welcome there. If not, I think the current position is that I have a preference to do it, and @Qard mildly disagrees but won't block it, so I think we're leaning towards deprecation there unless anybody else objects.

@pimterry is this ready to land?? i think its 4 days and counting now.....

[Qard]

Needed another approval first.

[nodejs-github-bot]

Landed in 8df6332

[navedjuwale-creator]

b0a79b1