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

PR_021

@MitchLewis930
Copy link
Author

@greptileai

@greptile-apps
Copy link

greptile-apps bot commented Jan 29, 2026

Greptile Overview

Greptile Summary

This PR adds experimental HTTP2 support to the http adapter with session pooling and reuse capabilities. The implementation introduces a new httpVersion config option (1 or 2) and http2Options for HTTP2-specific settings including a configurable sessionTimeout (default 1000ms).

Key changes:

  • Implemented Http2Sessions class for managing and reusing HTTP2 sessions by authority and options
  • Created http2Transport adapter that translates HTTP1-style options to HTTP2 headers and streams
  • Refactored abort handling to support both HTTP1 (req.destroy()) and HTTP2 (req.close()) request termination
  • Updated data writing logic from req.end(data) to req.write(data); req.end() for better compatibility
  • Changed settle(resolve, reject, response) to settle(resolve, abort, response) for stream responses
  • Added comprehensive test suite covering session reuse, timeouts, cancellation, and various response types
  • Updated TypeScript definitions and documentation

Issues found:

  • Default port in HTTP2 transport uses 80 instead of 443, which is incorrect for HTTPS-based HTTP2 connections

Confidence Score: 3/5

  • This PR is mostly safe but requires a fix to the default port logic before merging
  • The HTTP2 implementation is well-structured with comprehensive tests, but there's a critical logic error in the default port selection (line 239 of lib/adapters/http.js) that could cause connection failures. The code uses port 80 as default for HTTP2, but HTTP2 is typically used over HTTPS which requires port 443. This would cause issues when the port is not explicitly specified in the config.
  • lib/adapters/http.js requires attention for the default port logic fix

Important Files Changed

Filename Overview
README.md added HTTP2 documentation section with usage examples
index.d.ts added TypeScript type definitions for httpVersion and http2Options config options
lib/adapters/http.js implemented HTTP2 transport with session pooling, fixed abort handling, changed data writing logic (potential issues with default port logic)
test/unit/adapters/http.js added comprehensive HTTP2 test suite including session reuse, cancellation, timeouts, response types, and FormData support

Sequence Diagram

sequenceDiagram
    participant Client
    participant httpAdapter
    participant Http2Sessions
    participant http2Transport
    participant HTTP2Session
    participant Server

    Client->>httpAdapter: axios.request({httpVersion: 2})
    httpAdapter->>httpAdapter: validate httpVersion (1 or 2)
    httpAdapter->>httpAdapter: setup abort handlers
    
    alt HTTP2 Mode
        httpAdapter->>http2Transport: transport.request(options, callback)
        http2Transport->>http2Transport: build authority (protocol://hostname:port)
        http2Transport->>Http2Sessions: getSession(authority, http2Options)
        
        alt Session exists and valid
            Http2Sessions->>Http2Sessions: check existing sessions
            Http2Sessions->>Http2Sessions: match by authority & options
            Http2Sessions-->>http2Transport: return existing session
        else Create new session
            Http2Sessions->>HTTP2Session: connect(authority, options)
            HTTP2Session-->>Http2Sessions: new session
            Http2Sessions->>HTTP2Session: setTimeout(sessionTimeout)
            Http2Sessions->>Http2Sessions: store session
            Http2Sessions-->>http2Transport: return new session
        end
        
        http2Transport->>http2Transport: build HTTP2 headers
        http2Transport->>HTTP2Session: session.request(http2Headers)
        HTTP2Session->>Server: HTTP2 stream request
        
        Server-->>HTTP2Session: response headers
        HTTP2Session-->>http2Transport: emit 'response' event
        http2Transport->>http2Transport: set response.statusCode
        http2Transport-->>httpAdapter: callback(response)
        
        httpAdapter->>httpAdapter: process response stream
        Server-->>HTTP2Session: response data chunks
        HTTP2Session-->>httpAdapter: data chunks
        httpAdapter->>httpAdapter: decompress if needed
        httpAdapter->>httpAdapter: handle progress events
        
        alt responseType: 'stream'
            httpAdapter-->>Client: resolve with stream
        else buffer response
            httpAdapter->>httpAdapter: collect chunks
            httpAdapter->>httpAdapter: convert to responseType
            httpAdapter-->>Client: resolve with data
        end
        
        alt Session timeout
            HTTP2Session->>HTTP2Session: timeout after sessionTimeout ms
            HTTP2Session->>HTTP2Session: close()
            HTTP2Session->>Http2Sessions: emit 'close'
            Http2Sessions->>Http2Sessions: remove from sessions
        end
    else HTTP1 Mode
        httpAdapter->>Server: http/https.request()
        Server-->>httpAdapter: response
        httpAdapter-->>Client: resolve with response
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile


const http2Transport = {
request(options, cb) {
const authority = options.protocol + '//' + options.hostname + ':' + (options.port || 80);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default port should be 443 for HTTPS (HTTP2 is typically over HTTPS)

Suggested change
const authority = options.protocol + '//' + options.hostname + ':' + (options.port || 80);
const authority = options.protocol + '//' + options.hostname + ':' + (options.port || 443);
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/adapters/http.js
Line: 239:239

Comment:
default port should be 443 for HTTPS (HTTP2 is typically over HTTPS)

```suggestion
      const authority = options.protocol + '//' + options.hostname + ':' + (options.port || 443);
```

How can I resolve this? If you propose a fix, please make it concise.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants