Allow devs to supply a dispatcher to the Node implementation of fetch (#2178)

steveluscher · web-flow · commit a2fc5a3fda25 · 2024-02-27T10:58:13.000-08:00
# Summary The current version of web3.js allows folks to supply what's called an ‘HTTP Agent.’ This is a class that mediates how network requests should be fetched. One example is to establish a connection pool for a given URL, and keep connections open for a time so that TLS/SSL negotiation doesn't have to be done on every request. The new web3.js up until this point has punted on how to reintroduce this concept. In this PR we bring it back in the form of allowing callers to create transports with custom Undici `Dispatchers`. It only works in Node. If you supply the `dispatcher_NODE_ONLY` property in non-Node environments you'll get a console warning that your config has been ignored. # Alternate designs considered I desperately wanted to avoid the situation in this PR, which was to create a config property on transports that's only usable in Node. Alternate approaches considered: 1. Create a special transport for use in Node. Callers would have to `createDefaultNodeRpcTransport()` and/or `solanaRpcFactoryForNode()` which I thought was a bridge too far. 2. Encourage people to [inject a global dispatcher](https://stackoverflow.com/a/77526783/802047). This is a terrible idea for all of the reasons that global state is terrible: * Applies to all connections in the process * Your global dispatcher can be unset by someone _else_ * There might already be a global dispatcher installed (eg. a `ProxyAgent`) and you might unknowingly replace it 3. Export a `Dispatcher` setter that lets you install a dispatcher accessible _only_ to `@solana/fetch-impl`. Besides having most of the bad features of #&#8203;2, this would not work. We inline `@solana/fetch-impl` in the build, meaning there's no longer anywhere to export such a method from. # Test Plan ```shell cd packages/fetch-impl/ pnpm test:unit:node cd ../rpc-transport-http/ pnpm test:unit:node ``` See benchmark tests in next PR. Closes #2126.
diff --git a/packages/build-scripts/getBaseConfig.ts b/packages/build-scripts/getBaseConfig.ts
@@ -43,6 +43,8 @@ export function getBaseConfig(platform: Platform, formats: Format[], _options: O
                           external: [
                               // Despite inlining `@solana/text-encoding-impl`, do not recursively inline `fastestsmallesttextencoderdecoder`.
                               'fastestsmallesttextencoderdecoder',
+                              // Despite inlining `@solana/fetch-impl`, do not recursively inline `undici`.
+                              'undici',
                               // Despite inlining `@solana/ws-impl`, do not recursively inline `ws`.
                               'ws',
                           ],
diff --git a/packages/fetch-impl/package.json b/packages/fetch-impl/package.json
@@ -35,12 +35,17 @@
         "test:prettier": "jest -c node_modules/@solana/test-config/jest-prettier.config.ts --rootDir . --silent",
         "test:treeshakability:browser": "agadoo dist/index.browser.js",
         "test:treeshakability:node": "agadoo dist/index.node.js",
-        "test:typecheck": "tsc --noEmit"
+        "test:typecheck": "tsc --noEmit",
+        "test:unit:browser": "jest -c node_modules/@solana/test-config/jest-unit.config.browser.ts --rootDir . --silent --passWithNoTests",
+        "test:unit:node": "jest -c node_modules/@solana/test-config/jest-unit.config.node.ts --rootDir . --silent"
     },
     "browserslist": [
         "supports bigint and not dead",
         "maintained node versions"
     ],
+    "dependencies": {
+        "undici": "^6.2.2"
+    },
     "devDependencies": {
         "@solana/eslint-config-solana": "^1.0.2",
         "@solana/test-config": "workspace:*",
diff --git a/packages/fetch-impl/src/__tests__/fetch-test.node.ts b/packages/fetch-impl/src/__tests__/fetch-test.node.ts
@@ -0,0 +1,21 @@
+import { Dispatcher, fetch as fetchImpl } from 'undici';
+
+import fetch from '../index.node';
+
+jest.mock('undici');
+
+describe('fetch', () => {
+    it('should call the underlying `fetch` with the `dispatcher` supplied in `requestInit`', () => {
+        const explicitDispatcher = Symbol('explicitDispatcher') as unknown as Dispatcher;
+        fetch('http://solana.com', { dispatcher: explicitDispatcher });
+        expect(fetchImpl).toHaveBeenCalledWith('http://solana.com', {
+            dispatcher: explicitDispatcher,
+        });
+    });
+    it('should call the underlying `fetch` with an undefined `dispatcher` when an undefined is explicitly supplied in `requestInit`', () => {
+        fetch('http://solana.com', { dispatcher: undefined });
+        expect(fetchImpl).toHaveBeenCalledWith('http://solana.com', {
+            dispatcher: undefined,
+        });
+    });
+});
diff --git a/packages/fetch-impl/src/index.node.ts b/packages/fetch-impl/src/index.node.ts
@@ -1,2 +1 @@
-// TODO(https://github.com/solana-labs/solana-web3.js/issues/1787) Write HTTP/2 implementation.
-export default globalThis.fetch; // The Fetch API is supported natively in Node 18+.
+export { fetch as default } from 'undici';
diff --git a/packages/rpc-transport-http/package.json b/packages/rpc-transport-http/package.json
@@ -63,7 +63,8 @@
         "maintained node versions"
     ],
     "dependencies": {
-        "@solana/rpc-spec": "workspace:*"
+        "@solana/rpc-spec": "workspace:*",
+        "undici": "^6.6.2"
     },
     "devDependencies": {
         "@solana/build-scripts": "workspace:*",
diff --git a/packages/rpc-transport-http/src/__tests__/http-transport-dispatcher-test.browser.ts b/packages/rpc-transport-http/src/__tests__/http-transport-dispatcher-test.browser.ts
@@ -0,0 +1,53 @@
+import type { Dispatcher } from 'undici';
+
+const WARNING_MESSAGE =
+    'You have supplied a `Dispatcher` to `createHttpTransport()`. It has been ignored because ' +
+    'Undici dispatchers only work in Node environments. To eliminate this warning, omit the ' +
+    '`dispatcher_NODE_ONLY` property from your config when running in a non-Node environment.';
+
+describe('createHttpTransport()', () => {
+    let createHttpTransport: typeof import('../http-transport').createHttpTransport;
+    beforeEach(async () => {
+        jest.spyOn(console, 'warn').mockImplementation();
+        await jest.isolateModulesAsync(async () => {
+            createHttpTransport =
+                // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+                // @ts-ignore
+                (await import('../http-transport')).createHttpTransport;
+        });
+    });
+    describe('in development mode', () => {
+        beforeEach(() => {
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            (globalThis as any).__DEV__ = true;
+        });
+        it('warns when configured with a dispatcher', () => {
+            createHttpTransport({ dispatcher_NODE_ONLY: {} as Dispatcher, url: 'fake://url' });
+            expect(console.warn).toHaveBeenCalledWith(WARNING_MESSAGE);
+        });
+        it('warns when configured with an undefined dispatcher', () => {
+            createHttpTransport({ dispatcher_NODE_ONLY: undefined, url: 'fake://url' });
+            expect(console.warn).toHaveBeenCalledWith(WARNING_MESSAGE);
+        });
+        it('only warns once no matter how many times it is configured with a dispatcher', () => {
+            createHttpTransport({ dispatcher_NODE_ONLY: undefined, url: 'fake://url' });
+            createHttpTransport({ dispatcher_NODE_ONLY: {} as Dispatcher, url: 'fake://url' });
+            createHttpTransport({ dispatcher_NODE_ONLY: null as unknown as Dispatcher, url: 'fake://url' });
+            expect(console.warn).toHaveBeenCalledTimes(1);
+        });
+    });
+    describe('in production mode', () => {
+        beforeEach(() => {
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            (globalThis as any).__DEV__ = false;
+        });
+        it('does not warn when configured with a dispatcher', () => {
+            createHttpTransport({ dispatcher_NODE_ONLY: {} as Dispatcher, url: 'fake://url' });
+            expect(console.warn).not.toHaveBeenCalledWith(WARNING_MESSAGE);
+        });
+        it('does not warn when configured with an undefined dispatcher', () => {
+            createHttpTransport({ dispatcher_NODE_ONLY: undefined, url: 'fake://url' });
+            expect(console.warn).not.toHaveBeenCalledWith(WARNING_MESSAGE);
+        });
+    });
+});
diff --git a/packages/rpc-transport-http/src/__tests__/http-transport-headers-test.ts b/packages/rpc-transport-http/src/__tests__/http-transport-headers-test.ts
@@ -75,7 +75,7 @@ describe('createHttpRequest with custom headers', () => {
     it('is impossible to override the `Accept` header', () => {
         const makeHttpRequest = createHttpTransport({
             headers: { aCcEpT: 'text/html' },
-            url: 'fake://url',
+            url: 'http://localhost',
         });
         makeHttpRequest({ payload: 123 });
         expect(fetchImpl).toHaveBeenCalledWith(
@@ -90,7 +90,7 @@ describe('createHttpRequest with custom headers', () => {
     it('is impossible to override the `Content-Length` header', () => {
         const makeHttpRequest = createHttpTransport({
             headers: { 'cOnTeNt-LeNgTh': '420' },
-            url: 'fake://url',
+            url: 'http://localhost',
         });
         makeHttpRequest({ payload: 123 });
         expect(fetchImpl).toHaveBeenCalledWith(
@@ -105,7 +105,7 @@ describe('createHttpRequest with custom headers', () => {
     it('is impossible to override the `Content-Type` header', () => {
         const makeHttpRequest = createHttpTransport({
             headers: { 'cOnTeNt-TyPe': 'text/html' },
-            url: 'fake://url',
+            url: 'http://localhost',
         });
         makeHttpRequest({ payload: 123 });
         expect(fetchImpl).toHaveBeenCalledWith(
@@ -123,7 +123,7 @@ describe('createHttpRequest with custom headers', () => {
             createTransportWithForbiddenHeaders = () =>
                 createHttpTransport({
                     headers: { 'sEc-FeTcH-mOdE': 'no-cors' },
-                    url: 'fake://url',
+                    url: 'http://localhost',
                 });
         });
         it('throws in dev mode', () => {
diff --git a/packages/rpc-transport-http/src/__tests__/http-transport-test.ts b/packages/rpc-transport-http/src/__tests__/http-transport-test.ts
@@ -3,27 +3,26 @@ import { RpcTransport } from '@solana/rpc-spec';
 describe('createHttpTransport', () => {
     let fetchMock: jest.Mock;
     let makeHttpRequest: RpcTransport;
-    let oldFetch: typeof globalThis.fetch;
     let SolanaHttpError: typeof import('../http-transport-errors').SolanaHttpError;
     beforeEach(async () => {
-        oldFetch = globalThis.fetch;
-        globalThis.fetch = fetchMock = jest.fn();
         await jest.isolateModulesAsync(async () => {
-            const [{ createHttpTransport }, HttpTransportErrorsModule] = await Promise.all([
+            jest.mock('@solana/fetch-impl');
+            const [fetchImplModule, { createHttpTransport }, HttpTransportErrorsModule] = await Promise.all([
+                // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+                // @ts-ignore
+                import('@solana/fetch-impl'),
                 // eslint-disable-next-line @typescript-eslint/ban-ts-comment
                 // @ts-ignore
                 import('../http-transport'),
                 // eslint-disable-next-line @typescript-eslint/ban-ts-comment
                 // @ts-ignore
                 import('../http-transport-errors'),
             ]);
+            fetchMock = jest.mocked(fetchImplModule.default);
             SolanaHttpError = HttpTransportErrorsModule.SolanaHttpError;
-            makeHttpRequest = createHttpTransport({ url: 'fake://url' });
+            makeHttpRequest = createHttpTransport({ url: 'http://localhost' });
         });
     });
-    afterEach(() => {
-        globalThis.fetch = oldFetch;
-    });
     describe('when the endpoint returns a non-200 status code', () => {
         beforeEach(() => {
             fetchMock.mockResolvedValue({
@@ -58,7 +57,7 @@ describe('createHttpTransport', () => {
         });
         it('calls fetch with the specified URL', () => {
             makeHttpRequest({ payload: 123 });
-            expect(fetchMock).toHaveBeenCalledWith('fake://url', expect.anything());
+            expect(fetchMock).toHaveBeenCalledWith('http://localhost', expect.anything());
         });
         it('sets the `body` to a stringfied version of the payload', () => {
             makeHttpRequest({ payload: { ok: true } });
diff --git a/packages/rpc-transport-http/src/http-transport.ts b/packages/rpc-transport-http/src/http-transport.ts
@@ -1,5 +1,6 @@
 import fetchImpl from '@solana/fetch-impl';
 import { RpcTransport } from '@solana/rpc-spec';
+import type Dispatcher from 'undici/types/dispatcher';
 
 import { SolanaHttpError } from './http-transport-errors';
 import {
@@ -9,21 +10,45 @@ import {
 } from './http-transport-headers';
 
 type Config = Readonly<{
+    dispatcher_NODE_ONLY?: Dispatcher;
     headers?: AllowedHttpRequestHeaders;
     url: string;
 }>;
 
-export function createHttpTransport({ headers, url }: Config): RpcTransport {
+let didWarnDispatcherWasSuppliedInNonNodeEnvironment = false;
+function warnDispatcherWasSuppliedInNonNodeEnvironment() {
+    if (didWarnDispatcherWasSuppliedInNonNodeEnvironment) {
+        return;
+    }
+    didWarnDispatcherWasSuppliedInNonNodeEnvironment = true;
+    console.warn(
+        'You have supplied a `Dispatcher` to `createHttpTransport()`. It has been ignored ' +
+            'because Undici dispatchers only work in Node environments. To eliminate this ' +
+            'warning, omit the `dispatcher_NODE_ONLY` property from your config when running in ' +
+            'a non-Node environment.',
+    );
+}
+
+export function createHttpTransport(config: Config): RpcTransport {
+    if (__DEV__ && !__NODEJS__ && 'dispatcher_NODE_ONLY' in config) {
+        warnDispatcherWasSuppliedInNonNodeEnvironment();
+    }
+    const { headers, url } = config;
     if (__DEV__ && headers) {
         assertIsAllowedHttpRequestHeaders(headers);
     }
+    let dispatcherConfig: { dispatcher: Dispatcher | undefined } | undefined;
+    if (__NODEJS__ && 'dispatcher_NODE_ONLY' in config) {
+        dispatcherConfig = { dispatcher: config.dispatcher_NODE_ONLY };
+    }
     const customHeaders = headers && normalizeHeaders(headers);
     return async function makeHttpRequest<TResponse>({
         payload,
         signal,
     }: Parameters<RpcTransport>[0]): Promise<TResponse> {
         const body = JSON.stringify(payload);
         const requestInfo = {
+            ...dispatcherConfig,
             body,
             headers: {
                 ...customHeaders,
diff --git a/packages/rpc/src/rpc-transport.ts b/packages/rpc/src/rpc-transport.ts
@@ -6,10 +6,10 @@ import { RpcTransportFromClusterUrl } from './rpc-clusters';
 import { getRpcTransportWithRequestCoalescing } from './rpc-request-coalescer';
 import { getSolanaRpcPayloadDeduplicationKey } from './rpc-request-deduplication';
 
-type Config<TClusterUrl extends ClusterUrl> = Readonly<{
-    headers?: Parameters<typeof createHttpTransport>[0]['headers'];
+type RpcTransportConfig = Parameters<typeof createHttpTransport>[0];
+interface Config<TClusterUrl extends ClusterUrl> extends RpcTransportConfig {
     url: TClusterUrl;
-}>;
+}
 
 /**
  * Lowercasing header names makes it easier to override user-supplied headers.
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1 @@`
`1`		`-// TODO(https://github.com/solana-labs/solana-web3.js/issues/1787) Write HTTP/2 implementation.`
`2`		`-export default globalThis.fetch; // The Fetch API is supported natively in Node 18+.`
	`1`	`+export { fetch as default } from 'undici';`