Verified Commit 23b16d50 authored by Juhee Lee's avatar Juhee Lee Committed by GitLab
Browse files

chore: re-enable HTTP keep-alive with dynamic Node version check

parent fe4f2687

src/common/gitlab/api/api_client.test.ts

+117 −11
Original line number Diff line number Diff line
@@ -39,7 +39,7 @@ describe('DefaultApiClient', () => { 
    );

  });



  describe('default', () => {

  describe('default (keepAlive: false)', () => {

    beforeEach(() => {

      subject = new DefaultApiClient({

        instanceUrl: TEST_INSTANCE_URL,
@@ -72,8 +72,6 @@ describe('DefaultApiClient', () => { 
          agent: TEST_AGENT,

          headers: {

            Authorization: 'test 123456',

            // TODO: Re-enable once VS Code ships Node 24.18.0+

            // Connection: 'keep-alive',

            'User-Agent': 'test',

            'X-Test': '123',

          },
@@ -102,8 +100,6 @@ describe('DefaultApiClient', () => { 
          body: JSON.stringify(body),

          headers: {

            Authorization: 'test 123456',

            // TODO: Re-enable once VS Code ships Node 24.18.0+

            // Connection: 'keep-alive',

            'Content-Type': 'application/json',

            'User-Agent': 'test',

            'X-Test': '123',
@@ -163,8 +159,6 @@ describe('DefaultApiClient', () => { 
          }),

          headers: {

            Authorization: 'test 123456',

            // TODO: Re-enable once VS Code ships Node 24.18.0+

            // Connection: 'keep-alive',

            'Content-Type': 'application/json',

            'User-Agent': 'test',

          },
@@ -185,8 +179,6 @@ describe('DefaultApiClient', () => { 
        expect(connectToCable).toHaveBeenCalledWith(TEST_INSTANCE_URL, {

          headers: {

            Authorization: 'test 123456',

            // TODO: Re-enable once VS Code ships Node 24.18.0+

            // Connection: 'keep-alive',

            'User-Agent': 'test',

            Origin: TEST_INSTANCE_URL,

          },
@@ -223,6 +215,122 @@ describe('DefaultApiClient', () => { 
    });

  });



  describe('with keepAlive: true', () => {

    beforeEach(() => {

      subject = new DefaultApiClient({

        instanceUrl: TEST_INSTANCE_URL,

        agent: TEST_AGENT,

        headers: TEST_HEADERS,

        authProvider: TEST_AUTH_PROVIDER,

        keepAlive: true,

      });

    });



    it('sends Connection: keep-alive header on GET requests', async () => {

      await subject.fetchFromApi<unknown>({

        type: 'rest',

        method: 'GET',

        path: '/test',

      });



      expect(fetch).toHaveBeenCalledWith(

        expect.any(String),

        expect.objectContaining({

          headers: expect.objectContaining({

            Connection: 'keep-alive',

          }),

        }),

      );

    });



    it('sends Connection: keep-alive header on POST requests', async () => {

      await subject.fetchFromApi<unknown>({

        type: 'rest',

        method: 'POST',

        path: '/test',

        body: {},

      });



      expect(fetch).toHaveBeenCalledWith(

        expect.any(String),

        expect.objectContaining({

          headers: expect.objectContaining({

            Connection: 'keep-alive',

          }),

        }),

      );

    });



    it('sends Connection: keep-alive header on graphql requests', async () => {

      const responseData = { data: { test: 'FOO' } };

      jest.mocked(fetch).mockResolvedValue(

        createFakeResponse({

          text: Promise.resolve(JSON.stringify(responseData)),

          headers: { 'Content-Type': 'application/json' },

        }),

      );



      await subject.fetchFromApi({

        type: 'graphql',

        query: 'query { test }',

        variables: {},

      });



      expect(fetch).toHaveBeenCalledWith(

        expect.any(String),

        expect.objectContaining({

          headers: expect.objectContaining({

            Connection: 'keep-alive',

          }),

        }),

      );

    });



    it('sends Connection: keep-alive header when connecting to cable', async () => {

      jest.mocked(connectToCable).mockResolvedValue(createFakePartial<Cable>({}));



      await subject.connectToCable();



      expect(connectToCable).toHaveBeenCalledWith(

        TEST_INSTANCE_URL,

        expect.objectContaining({

          headers: expect.objectContaining({

            Connection: 'keep-alive',

          }),

        }),

      );

    });

  });



  describe('with keepAlive: false (explicit)', () => {

    beforeEach(() => {

      subject = new DefaultApiClient({

        instanceUrl: TEST_INSTANCE_URL,

        agent: TEST_AGENT,

        headers: TEST_HEADERS,

        authProvider: TEST_AUTH_PROVIDER,

        keepAlive: false,

      });

    });



    it('does not send Connection: keep-alive header', async () => {

      await subject.fetchFromApi<unknown>({

        type: 'rest',

        method: 'GET',

        path: '/test',

      });



      expect(fetch).toHaveBeenCalledWith(

        expect.any(String),

        expect.objectContaining({

          headers: expect.not.objectContaining({

            Connection: 'keep-alive',

          }),

        }),

      );

    });

  });



  describe('with no auth provider or agent', () => {

    beforeEach(() => {

      subject = new DefaultApiClient({
@@ -247,8 +355,6 @@ describe('DefaultApiClient', () => { 


      expect(fetch).toHaveBeenCalledWith(`${TEST_INSTANCE_URL}/api/v4/test?param=123&foo=bar`, {

        headers: {

          // TODO: Re-enable once VS Code ships Node 24.18.0+

          // Connection: 'keep-alive',

          'User-Agent': 'test',

          'X-Test': '123',

        },

src/common/gitlab/api/api_client.ts

+14 −4
Original line number Diff line number Diff line
@@ -24,6 +24,14 @@ export interface DefaultApiClientOptions { 
  authProvider?: AuthProvider;

  headers?: Record<string, string>;

  agent?: HttpsProxyAgent<string> | https.Agent;

  /**

   * When true, sends the `Connection: keep-alive` header on each request.

   * This works around a VS Code proxy issue (https://github.com/microsoft/vscode/issues/173861)

   * where the header is clobbered regardless of `http.proxySupport` settings.

   * Should be set to false on Node 24.17.x due to a keep-alive regression

   * (https://github.com/nodejs/node/issues/63989).

   */

  keepAlive?: boolean;

}



export const NOOP_AUTH_PROVIDER: AuthProvider = {
@@ -39,11 +47,14 @@ export class DefaultApiClient implements ApiClient { 


  readonly #agent?: HttpsProxyAgent<string> | https.Agent;



  readonly #keepAlive: boolean;



  constructor(options: DefaultApiClientOptions) {

    this.#instanceUrl = options.instanceUrl;

    this.#authProvider = options.authProvider || NOOP_AUTH_PROVIDER;

    this.#headers = options.headers || {};

    this.#agent = options.agent;

    this.#keepAlive = options.keepAlive ?? false;

  }



  async fetchFromApi<T>(request: ApiRequest<T>): Promise<T> {
@@ -182,10 +193,9 @@ export class DefaultApiClient implements ApiClient { 
        // Setting this header normally isn't necessary if the HTTP agent has

        // keepAlive: true set, but due to https://github.com/microsoft/vscode/issues/173861

        // something is clobbering this header no matter how `http.proxySupport` is set.

        // TODO: Re-enable once VS Code ships Node 24.18.0+

        // keepAlive is disabled to work around a Node.js regression shipped in VS Code 1.128.0 (Node 24.17.0 / Electron 42.5.0).

        // See https://github.com/nodejs/node/issues/63989 and https://github.com/node-fetch/node-fetch/issues/1767

        // Connection: 'keep-alive',

        // The header is only sent when keepAlive is enabled (i.e. not on Node 24.17.x,

        // which has a keep-alive regression: https://github.com/nodejs/node/issues/63989).

        ...(this.#keepAlive ? { Connection: 'keep-alive' } : {}),

        ...authorizationHeaders,

        ...this.#headers,

      },

src/desktop/gitlab/gitlab_service.ts

+2 −0
Original line number Diff line number Diff line
@@ -49,6 +49,7 @@ import { GqlBasePosition, GqlGenericNote, GqlNote, Node } from './graphql/shared 
import { getMrPermissionsQuery, MrPermissionsQueryOptions } from './graphql/mr_permission';

import { getHttpAgentOptions } from './http/get_http_agent_options';

import { getUserAgentHeader } from './http/get_user_agent_header';

import { isKeepAliveAffectedNodeVersion } from './http/is_keep_alive_affected_node_version';

import { ensureAbsoluteAvatarUrl } from './ensure_absolute_avatar_url';



interface CreateNoteResult {
@@ -197,6 +198,7 @@ const getDefaultApiClientOptions = (instanceUrl: string): DefaultApiClientOption 
  instanceUrl,

  agent: getHttpAgent(instanceUrl),

  headers: getUserAgentHeader(),

  keepAlive: !isKeepAliveAffectedNodeVersion(),

});



export class GitLabService {

src/desktop/gitlab/http/get_http_agent_options.ts

+4 −3
Original line number Diff line number Diff line
@@ -2,6 +2,7 @@ import * as fs from 'fs'; 
import * as vscode from 'vscode';

import { UserFriendlyError } from '../../../common/errors/user_friendly_error';

import { handleError } from '../../../common/errors/handle_error';

import { isKeepAliveAffectedNodeVersion } from './is_keep_alive_affected_node_version';



export interface GitLabHttpAgentOptions {

  ca?: Buffer;
@@ -13,10 +14,10 @@ export interface GitLabHttpAgentOptions { 
}



export const getHttpAgentOptions = (): GitLabHttpAgentOptions => {

  // TODO: Re-enable once VS Code ships Node 24.18.0+

  // keepAlive is disabled to work around a Node.js regression shipped in VS Code 1.128.0 (Node 24.17.0 / Electron 42.5.0).

  // Disable keep-alive only for Node 24.17.x, which has a regression in http.Agent

  // keep-alive socket handling that breaks node-fetch v2 gzip/chunked responses.

  // See https://github.com/nodejs/node/issues/63989 and https://github.com/node-fetch/node-fetch/issues/1767

  const result: GitLabHttpAgentOptions = { keepAlive: false };

  const result: GitLabHttpAgentOptions = { keepAlive: !isKeepAliveAffectedNodeVersion() };

  // FIXME: if you are touching this configuration statement, move the configuration to extension_configuration.ts

  const { ignoreCertificateErrors, ca, cert, certKey } =

    vscode.workspace.getConfiguration('gitlab');

src/desktop/gitlab/http/is_keep_alive_affected_node_version.test.ts

0 → 100644
+38 −0
Original line number Diff line number Diff line 
import { isKeepAliveAffectedNodeVersion } from './is_keep_alive_affected_node_version';



describe('isKeepAliveAffectedNodeVersion', () => {

  const originalVersions = process.versions;



  afterEach(() => {

    Object.defineProperty(process, 'versions', {

      value: originalVersions,

      writable: true,

      configurable: true,

    });

  });



  const setNodeVersion = (version: string) => {

    Object.defineProperty(process, 'versions', {

      value: { ...originalVersions, node: version },

      writable: true,

      configurable: true,

    });

  };



  const cases: [version: string, affected: boolean, description: string][] = [

    ['24.17.0', true, 'the affected version'],

    ['24.17.1', true, 'a patch of the affected minor'],

    ['24.18.0', false, 'the fixed version'],

    ['24.16.0', false, 'before the regression'],

    ['22.17.0', false, 'unrelated major'],

    ['20.17.0', false, 'unrelated major'],

    ['25.0.0', false, 'future major'],

  ];



  cases.forEach(([version, affected, description]) => {

    it(`returns ${affected} for Node ${version} (${description})`, () => {

      setNodeVersion(version);

      expect(isKeepAliveAffectedNodeVersion()).toBe(affected);

    });

  });

});