http2: 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 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.74%. Comparing base (3ab9dd8) to head (a3b1324).
⚠️ Report is 40 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61713      +/-   ##
==========================================
+ Coverage   89.73%   89.74%   +0.01%     
==========================================
  Files         675      675              
  Lines      204502   204657     +155     
  Branches    39304    39326      +22     
==========================================
+ Hits       183502   183665     +163     
+ Misses      13283    13262      -21     
- Partials     7717     7730      +13     

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

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

Would be good to drop the now-unnecessary lines. Otherwise though I'm on board with this all LGTM 👍. Good quick work @amyssnippet! Nicely done.

I've added the 'notable change' label since it's now a significantly larger change than the original outline, and includes a deprecation. @amyssnippet once you've dropped the below, can you flatten this into one commit with an updated message that summarizes the whole change? This will end up in the changelog when it's released.

[pimterry]

storeHTTPOptions already sets these fields - now that we're using it, I think we can delete this code.

[github-actions]

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @pimterry.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[Qard]

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.

Fair point. I'm not really a fan of the structure of what we're putting into the http1Options, but it at least moves us a step away from the current state of having separate http1-related options directly in the parent object. Perhaps the depreciation is fine and we can just make another plan separately for what to do to make the http1Options contents more friendly?

[pimterry]

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]

Fair point. I'm not really a fan of the structure of what we're putting into the http1Options, but it at least moves us a step away from the current state of having separate http1-related options directly in the parent object. Perhaps the depreciation is fine and we can just make another plan separately for what to do to make the http1Options contents more friendly?

Hmm, I'm not sure what you mean about the 'structure' here. Is there something specific you have in mind to make this more friendly? Just to make sure we're leaving the space open so that those kinds changes are possible later, not boxing ourselves in.

[Qard]

Nothing specific, but IncomingMessage and ServerResponse are very much implementation details of the http1 code so it feels weird that we'd be naming public options after those. It also doesn't really follow our normal object property casing standards. Not really any different than it was previously though. My point is just that I feel like we should be thinking of a better interface for that.

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