Skip to content

http2: Fix issue where slow requests are terminated prematurely #9280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
hyperair wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

http2: Fix issue where slow requests are terminated prematurely #9280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ac0f8b
http2: Fix issue where slow requests are terminated prematurely
hyperair Jan 6, 2026
fd10d63
Fix http2 tests
hyperair Jan 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 19 additions & 23 deletions core/packages/nodejs-googleapis-common/src/http2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ const DEBUG = !!process.env.HTTP2_DEBUG;
*/
export interface SessionData {
session: http2.ClientHttp2Session;
timeoutHandle?: NodeJS.Timeout;
}
Comment on lines 39 to 41

@gemini-code-assist gemini-code-assist Bot Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

To safely manage the lifecycle of the HTTP/2 session without prematurely terminating slow requests, we should track the number of active requests on the session using a reference count (activeStreams) and keep the timeoutHandle to schedule the idle cleanup.

export interface SessionData {
  session: http2.ClientHttp2Session;
  timeoutHandle?: NodeJS.Timeout;
  activeStreams?: number;
}
References
  1. When implementing timeout or keepalive mechanisms, prefer precise timer mechanisms (like setTimeout) over periodic polling (like setInterval) for efficiency and precision.


/**
Expand Down Expand Up @@ -72,12 +71,6 @@ export async function request<T>(
// Check for an existing session to this host, or go create a new one.
const sessionData = _getClient(url.host);

// Since we're using this session, clear the timeout handle to ensure
// it stays in memory and connected for a while further.
if (sessionData.timeoutHandle !== undefined) {
clearTimeout(sessionData.timeoutHandle);
}

// Assemble the querystring based on config.params. We're using the
// `qs` module to make life a little easier.
let pathWithQs = url.pathname;
Expand Down Expand Up @@ -181,7 +174,7 @@ export async function request<T>(
return;
});
} catch (e) {
closeSession(url)
closeSession(url.host, session)
.then(() => reject(e))
.catch(reject);
return;
Expand All @@ -202,13 +195,6 @@ export async function request<T>(
req.end(data);
}
}

// Create a timeout so the Http2Session will be cleaned up after
// a period of non-use. 500 milliseconds was chosen because it's
// a nice round number, and I don't know what would be a better
// choice. Keeping this channel open will hold a file descriptor
// which will prevent the process from exiting.
sessionData.timeoutHandle = setTimeout(() => closeSession(url), 500);
});
}

Expand Down Expand Up @@ -238,6 +224,13 @@ function _getClient(host: string): SessionData {
.on('goaway', (errorCode, lastStreamId) => {
console.error(`*GOAWAY*: ${errorCode} : ${lastStreamId}`);
delete sessions[host];
})
.setTimeout(500, () => {
// Clean up Http2Session after a period of non-use. 500 milliseconds was
// chosen because it's a nice round number, and I don't know what would
// be a better choice. Keeping this channel open will hold a file
// descriptor which will prevent the process from exiting.
return closeSession(host, session);
});
sessions[host] = {session};
Comment on lines 224 to 235

@gemini-code-assist gemini-code-assist Bot Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Using session.setTimeout(500) is problematic because it triggers on socket inactivity. If a request is slow to respond (e.g., waiting for server-side processing), there is no data flowing, which will trigger this timeout, close the session, and eventually abort the active request after the 1-second grace period in closeSession.

Instead, we should wrap session.request to track active streams and only schedule the 500ms idle timeout when there are no active streams left.

      .on('goaway', (errorCode, lastStreamId) => {
        console.error('*GOAWAY*: ' + errorCode + ' : ' + lastStreamId);
        delete sessions[host];
      });
    const sessionData: SessionData = {session};
    const originalRequest = session.request;
    session.request = function (this: http2.ClientHttp2Session, ...args: any[]) {
      if (sessionData.timeoutHandle) {
        clearTimeout(sessionData.timeoutHandle);
        sessionData.timeoutHandle = undefined;
      }
      sessionData.activeStreams = (sessionData.activeStreams || 0) + 1;
      const req = originalRequest.apply(this, args as any);
      req.on('close', () => {
        sessionData.activeStreams = (sessionData.activeStreams || 1) - 1;
        if (sessionData.activeStreams === 0) {
          sessionData.timeoutHandle = setTimeout(() => {
            closeSession(host, session);
          }, 500);
        }
      });
      return req;
    } as any;
    sessions[host] = sessionData;
References
  1. When implementing timeout or keepalive mechanisms, prefer precise timer mechanisms (like setTimeout) over periodic polling (like setInterval) for efficiency and precision.

@hyperair hyperair Sep 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The intention here is to terminate connections that have hung as well, even if they still have active streams that may have stalled. Tracking active streams and keeping the connection alive until all streams are done would defeat that purpose.

} else {
Expand All @@ -248,25 +241,28 @@ function _getClient(host: string): SessionData {
return sessions[host];
}

export async function closeSession(url: URL) {
const sessionData = sessions[url.host];
if (!sessionData) {
export async function closeSession(
host: string,
session: http2.ClientHttp2Session,
) {
const sessionData = sessions[host];
if (sessionData?.session !== session) {
// session has been replaced with a different session, don't touch
return;
}
const {session} = sessionData;
delete sessions[url.host];
delete sessions[host];
if (DEBUG) {
console.error(`Closing ${url.host}`);
console.error(`Closing ${host}`);
}
session.close(() => {
if (DEBUG) {
console.error(`Closed ${url.host}`);
console.error(`Closed ${host}`);
}
});
setTimeout(() => {
if (session && !session.destroyed) {
if (DEBUG) {
console.log(`Forcing close ${url.host}`);
console.log(`Forcing close ${host}`);
}
if (session) {
session.destroy();
Expand Down
3 changes: 3 additions & 0 deletions core/packages/nodejs-googleapis-common/test/test.http2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ class FakeClient extends EventEmitter {
callback();
};
destroy = () => {};
setTimeout = (timeout: number, callback: () => {}) => {
setTimeout(callback, timeout);
};
Comment on lines +45 to +47

@gemini-code-assist gemini-code-assist Bot Sep 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Since we are no longer using session.setTimeout on the HTTP/2 session, this mock is no longer needed and can be removed.

@hyperair hyperair Sep 11, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is dependent on #9280 (comment), ignoring for now

}

describe('http2', () => {
Expand Down
Loading