Skip to content

Comments

feat(http): add HTTP2 support; (#7150)#1

Open
MitchLewis930 wants to merge 1 commit intopr_021_beforefrom
pr_021_after
Open

feat(http): add HTTP2 support; (#7150)#1
MitchLewis930 wants to merge 1 commit intopr_021_beforefrom
pr_021_after

Conversation

@MitchLewis930
Copy link

@MitchLewis930 MitchLewis930 commented Jan 30, 2026

User description

PR_021


PR Type

Enhancement


Description

  • Add HTTP/2 support with session management and reusable connections

  • Implement Http2Sessions class for session pooling and timeout handling

  • Add httpVersion and http2Options configuration parameters

  • Refactor abort handling and improve error management in request lifecycle

  • Extract server test utilities to shared helpers module

  • Add comprehensive HTTP/2 test suite with various response types and scenarios


Diagram Walkthrough

flowchart LR
  A["HTTP Adapter"] -->|"httpVersion=2"| B["HTTP/2 Transport"]
  B -->|"getSession"| C["Http2Sessions Manager"]
  C -->|"connect"| D["HTTP/2 Session Pool"]
  D -->|"reuse/timeout"| E["Session Lifecycle"]
  A -->|"httpVersion=1"| F["HTTP/1.x Transport"]
  G["Config: httpVersion, http2Options"] -->|"merge"| A
Loading

File Walkthrough

Relevant files
Enhancement
http.js
Implement HTTP/2 support with session management                 

lib/adapters/http.js

  • Import http2 module constants and implement Http2Sessions class for
    session pooling with timeout management
  • Add http2Transport object implementing HTTP/2 request handling with
    header mapping
  • Add httpVersion and http2Options configuration parameters with
    validation
  • Refactor abort handling to use abortEmitter instead of generic emitter
  • Replace direct reject() calls with abort() function for consistent
    error handling
  • Update response stream handling to properly manage stream lifecycle
  • Change request data writing to use req.write() instead of
    req.end(data)
  • Use utils.toFiniteNumber() for content-length parsing
+185/-43
server.js
Add HTTP/2 server support with self-signed certificates   

test/helpers/server.js

  • Add http2 and selfsigned module imports for HTTPS support
  • Generate self-signed certificates for HTTP/2 testing
  • Add useHTTP2, key, and cert parameters to server configuration
  • Create HTTP/2 secure server when useHTTP2 flag is enabled
  • Implement session tracking and closeAllSessions() method for HTTP/2
    servers
  • Conditionally set keepAliveTimeout only for HTTP/1.x servers
+45/-4   
Tests
http.js
Add HTTP/2 tests and refactor test utilities                         

test/unit/adapters/http.js

  • Extract startHTTPServer, stopHTTPServer, handleFormData, and
    generateReadable to shared helpers
  • Add server2 variable and HTTP/2 specific port constants
  • Fix toleranceRange() calculation logic for rate limit testing
  • Add comprehensive HTTP/2 test suite covering basic requests, payloads,
    FormData, response types, timeouts, cancellation, and session
    management
  • Update rate limit test parameters for improved stability
  • Refactor stream abort test to use assert.rejects() pattern
+424/-112
Documentation
index.d.ts
Add HTTP/2 type definitions                                                           

index.d.ts

  • Add httpVersion option typed as 1 | 2 for protocol version selection
  • Add http2Options configuration object with optional sessionTimeout
    parameter
+4/-0     
index.d.cts
Add HTTP/2 type definitions for CommonJS                                 

index.d.cts

  • Add httpVersion option typed as 1 | 2 for protocol version selection
  • Add http2Options configuration object with optional sessionTimeout
    parameter
+4/-0     
README.md
Document HTTP/2 feature and usage                                               

README.md

  • Add HTTP/2 section to table of contents
  • Document HTTP/2 support with usage example showing httpVersion and
    http2Options configuration
  • Explain sessionTimeout parameter and session reuse behavior
+29/-0   
Dependencies
package.json
Add selfsigned certificate dependency                                       

package.json

  • Add selfsigned dependency version ^3.0.1 for generating self-signed
    certificates in tests
+1/-0     

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: 🏷️
HTTP2 session cleanup bug: The session.once('close', ...) handler dereferences authoritySessions (via
entries = authoritySessions) which can be undefined for a newly-created authority, causing
a runtime error during session close and preventing graceful cleanup.

Referred Code
const session = connect(authority, options);

session.once('close', () => {
  let entries = authoritySessions, len = entries.length, i = len;

  while (i--) {
    if (entries[i][0] === session) {
      entries.splice(i, 1);
      if (len === 1) {
        delete this.sessions[authority];
        return;
      }
    }
  }
});

Http2Sessions.setTimeout(session, options.sessionTimeout);

let entries = this.sessions[authority], entry = [
  session,
  options


 ... (clipped 5 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: 🏷️
Unstructured sensitive logging: The new console.warn('emit error', err) introduces unstructured logging and may
log sensitive request/context data contained in err to application logs.

Referred Code
function abort(reason) {
  try {
    abortEmitter.emit('abort', !reason || reason.type ? new CanceledError(null, config, req) : reason);
  } catch(err) {
    console.warn('emit error', err);
  }
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: 🏷️
Console error exposure: The new console.warn('emit error', err) may surface internal error details
(potentially including stack traces) to end-users depending on how consumers handle
stdout/stderr.

Referred Code
function abort(reason) {
  try {
    abortEmitter.emit('abort', !reason || reason.type ? new CanceledError(null, config, req) : reason);
  } catch(err) {
    console.warn('emit error', err);
  }
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: 🏷️
Unvalidated http2Options passthrough: http2Options is passed through to http2.connect() without validation/sanitization, which
may allow insecure TLS/session settings unless constrained elsewhere in the codebase.

Referred Code
getSession(authority, options) {
  options = Object.assign({
    sessionTimeout: 1000
  }, options);

  let authoritySessions;

  if ((authoritySessions = this.sessions[authority])) {
    let len = authoritySessions.length;

    for (let i = 0; i < len; i++) {
      const [sessionHandle, sessionOptions] = authoritySessions[i];
      if (!sessionHandle.destroyed && !sessionHandle.closed && util.isDeepStrictEqual(sessionOptions, options)) {
        return sessionHandle;
      }
    }
  }

  const session = connect(authority, options);

  session.once('close', () => {

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Propagate HTTP/2 errors correctly

In http2Transport, add 'error' event listeners to both the HTTP/2 session and
the stream to ensure errors are propagated correctly through the adapter's error
handling.

lib/adapters/http.js [237-275]

 const http2Transport = {
   request(options, cb) {
-    const authority = options.protocol + '//' + options.hostname + ':' + (options.port || 80);
-
-    const {http2Options, headers} = options;
-    const session = http2Sessions.getSession(authority, http2Options);
-
-    const http2Headers = {
-      [HTTP2_HEADER_SCHEME]: options.protocol.replace(':', ''),
-      [HTTP2_HEADER_METHOD]: options.method,
-      [HTTP2_HEADER_PATH]: options.path,
-    }
-
-    utils.forEach(headers, (header, name) => {
-      name.charAt(0) !== ':' && (http2Headers[name] = header);
-    });
-
+    // ...header setup...
     const req = session.request(http2Headers);
 
-    req.once('response', (responseHeaders) => {
-      const response = req; //duplex
+    // propagate stream errors to the adapter
+    req.on('error', err => req.emit('error', err));
+    session.on('error', err => req.emit('error', err));
+
+    req.once('response', responseHeaders => {
+      const response = req;
       // header mapping...
       cb(response);
-    })
+    });
 
     return req;
   }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: This suggestion addresses a critical error handling gap by ensuring that errors from the underlying HTTP/2 session and stream are properly propagated, preventing unhandled exceptions.

Medium
Avoid mutating native stream object

Avoid mutating the native Http2Stream object in http2Transport. Instead, create
a new response object that inherits from the stream to attach properties like
headers and statusCode.

lib/adapters/http.js [258-270]

-const response = req; //duplex
+const response = Object.create(req); // duplex
 
 responseHeaders = Object.assign({}, responseHeaders);
 
 const status = responseHeaders[HTTP2_HEADER_STATUS];
 
 delete responseHeaders[HTTP2_HEADER_STATUS];
 
 response.headers = responseHeaders;
 
 response.statusCode = +status;
 
 cb(response);
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly points out that mutating a native Node.js Http2Stream object is poor practice and proposes a safer alternative using Object.create() to avoid direct mutation while preserving the prototype chain.

Low
Possible issue
Normalize and simplify session pooling

Refactor the Http2Sessions class to simplify session pooling. Normalize the
session array on initialization and reference the live pool in the 'close'
handler for more robust cleanup.

lib/adapters/http.js [65-121]

 class Http2Sessions {
   constructor() {
     this.sessions = Object.create(null);
   }
 
   getSession(authority, options) {
-    options = Object.assign({
-      sessionTimeout: 1000
-    }, options);
-
-    let authoritySessions;
-
-    if ((authoritySessions = this.sessions[authority])) {
-      // reuse logic...
-    }
+    options = Object.assign({ sessionTimeout: 1000 }, options);
+    // ensure an array exists for this authority
+    const pool = this.sessions[authority] || [];
 
     const session = connect(authority, options);
 
     session.once('close', () => {
-      let entries = authoritySessions, len = entries.length, i = len;
-      // cleanup logic...
+      const entries = this.sessions[authority] || [];
+      for (let i = entries.length - 1; i >= 0; i--) {
+        if (entries[i][0] === session) {
+          entries.splice(i, 1);
+          if (entries.length === 0) {
+            delete this.sessions[authority];
+          }
+          break;
+        }
+      }
     });
 
     Http2Sessions.setTimeout(session, options.sessionTimeout);
 
-    let entries = this.sessions[authority], entry = [
-      session,
-      options
-    ];
-
-    entries ? this.sessions[authority].push(entry) : authoritySessions =  this.sessions[authority] = [entry];
+    pool.push([session, options]);
+    this.sessions[authority] = pool;
 
     return session;
   }
 
   static setTimeout(session, timeout = 1000) {
-    session && session.setTimeout(timeout, () => {
-      session.close();
-    });
+    session.setTimeout(timeout, () => session.close());
   }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion offers a reasonable refactoring of the Http2Sessions class to simplify session cleanup logic, making it more robust and easier to read, although the current implementation is not incorrect.

Low
  • More

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants