Review requested:

• @nodejs/http2
• @nodejs/net

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.

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

@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 kindly rerun the failing check and also run the internal jenkins ci so that changes are verified to merge

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

@ronag is everything good?? all checks successfull

kindly review, if any changes then let me know

@ronag kindly run the internal jenkins ci once again

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.

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!

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

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

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 i have resolved the merge conflict, every check here is successful, so lets wait until someone else reviews and gives their comments. Thanks!

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

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

Landed in 8df6332