Repository: nodejs/node
File: lib/internal/http2/core.js (JavaScript)

Code before:
class Http2SecureServer extends TLSServer {
  constructor(options, requestListener) {
    options = initializeTLSOptions(options);
    super(options, connectionListener);
    this[kOptions] = options;
    this[kSessions] = new SafeSet();
    this.timeout = 0;
    this.on('newListener', setupCompat);
    if (options.allowHTTP1 === true) {
      this.headersTimeout = 60_000; // Minimum between 60 seconds or requestTimeout
      this.requestTimeout = 300_000; // 5 minutes
      this.connectionsCheckingInterval = 30_000; // 30 seconds
      const keepAliveTimeout = options.keepAliveTimeout;
      if (keepAliveTimeout !== undefined) {
        validateInteger(keepAliveTimeout, 'keepAliveTimeout', 0);
        this.keepAliveTimeout = keepAliveTimeout;
      } else {
        this.keepAliveTimeout = 5_000; // 5 seconds;
      }
      const keepAliveTimeoutBuffer = options.keepAliveTimeoutBuffer;
      if (keepAliveTimeoutBuffer !== undefined) {
        validateInteger(keepAliveTimeoutBuffer, 'keepAliveTimeoutBuffer', 0);
        this.keepAliveTimeoutBuffer = keepAliveTimeoutBuffer;
      } else {
        this.keepAliveTimeoutBuffer = 1000;
      }
      this.shouldUpgradeCallback = function() {
        return this.listenerCount('upgrade') > 0;
      };
      this.on('listening', setupConnectionsTracking);
    }
    if (typeof requestListener === 'function')
      this.on('request', requestListener);
    this.on('tlsClientError', onErrorSecureServerSession);
  }

  setTimeout(msecs, callback) {
    this.timeout = msecs;
    if (callback !== undefined) {
      validateFunction(callback, 'callback');
      this.on('timeout', callback);
    }
    return this;
  }

  updateSettings(settings) {
    assertIsObject(settings, 'settings');
    validateSettings(settings);
    this[kOptions].settings = { ...this[kOptions].settings, ...settings };
  }


Review comment (suggestion):
We've run into issues like this before with other new options like this, where they end up set only for normal HTTP/1 and not for HTTP/2 allowHttp1 (e.g. https://github.com/nodejs/node/pull/59924).

What would happen if we called HTTP1's `storeOptions` here directly somehow? That would give us a single place to define the HTTP/1 options that affects both cases.

Right now we're duplicating the options logic, occasionally breaking things along the way, and creating slightly inconsistent behaviour throughout: for example this new code supports `options.keepAliveTimeout` if set, but we don't use `options.headersTimeout` in the equivalent options above - we just use a fixed default. It would be great to solve these kinds of issues permanently & consistently.

Imo: if allowHttp1 is set, we could just support all the HTTP/1 options here, in the same way as for an HTTP/1 server. Unless there's some specific case that we can't handle or that's incompatible? I can't see one.

If we want to be explicit & avoid any risk of conflicts in future, we could use separate `http1Options` option for this (only used with `allowHttp1`, so let's throw if it's used without that):

```
http2.createServer({
  // ...Http/2 options
  allowHttp1: true,
  http1Options: {
    keepAliveTimeout: 60_000,
    incomingMessage: ...
  }
});
```

We would then doc-deprecate things like `options.http1IncomingMessage` that mirrors HTTP/1's `options.incomingMessage`, as people could use `options.http1Options.incomingMessage` instead.

This is a bit more complicated and needs some thought & testing, but I think there's a reasonable solution that's reachable there and it would be great to stop patching the same problem over & over again.

Code after:
}

/**
 * This function closes all active sessions gracefully.
 * @param {*} server the underlying server whose sessions to be closed
 */
function closeAllSessions(server) {
  const sessions = server[kSessions];
  if (sessions.size > 0) {
    for (const session of sessions) {
      session.close();
    }
  }
}

class Http2SecureServer extends TLSServer {
  constructor(options, requestListener) {
    options = initializeTLSOptions(options);
    super(options, connectionListener);
    this[kOptions] = options;
    this[kSessions] = new SafeSet();
    this.timeout = 0;
    this.on('newListener', setupCompat);
    if (options.allowHTTP1 === true) {
      storeHTTPOptions.call(this, { ...options, ...options.http1Options });
      this.shouldUpgradeCallback = function() {
        return this.listenerCount('upgrade') > 0;
      };
      this.on('listening', setupConnectionsTracking);
    }
    if (typeof requestListener === 'function')
      this.on('request', requestListener);
    this.on('tlsClientError', onErrorSecureServerSession);
  }

  setTimeout(msecs, callback) {
    this.timeout = msecs;
    if (callback !== undefined) {
      validateFunction(callback, 'callback');
      this.on('timeout', callback);
    }
    return this;
  }

  updateSettings(settings) {
    assertIsObject(settings, 'settings');
    validateSettings(settings);
    this[kOptions].settings = { ...this[kOptions].settings, ...settings };
  }

  close() {