Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix socket info in fetch response #555

Merged
merged 3 commits into from
Dec 4, 2024
Merged

fix: fix socket info in fetch response #555

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe689a4
fix: fix socket info in response
killagu Dec 4, 2024
457fa29
f
killagu Dec 4, 2024
b7a9e61
f
killagu Dec 4, 2024
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
27 changes: 27 additions & 0 deletions src/BaseAgent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import {
Agent,
Dispatcher,
} from 'undici';
import { AsyncLocalStorage } from 'node:async_hooks';
import { FetchOpaque } from './FetchOpaqueInterceptor.js';

export interface BaseAgentOptions extends Agent.Options {
opaqueLocalStorage?: AsyncLocalStorage<FetchOpaque>;
}

export class BaseAgent extends Agent {
#opaqueLocalStorage?: AsyncLocalStorage<FetchOpaque>;

constructor(options: BaseAgentOptions) {
super(options);
this.#opaqueLocalStorage = options.opaqueLocalStorage;
}

dispatch(options: Agent.DispatchOptions, handler: Dispatcher.DispatchHandler): boolean {
const opaque = this.#opaqueLocalStorage?.getStore();
if (opaque) {
(handler as any).opaque = opaque;
}
return super.dispatch(options, handler);
}
Comment on lines +20 to +26
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve type safety and error handling in dispatch

The current implementation has several areas for improvement:

  1. Using any type assertion is unsafe
  2. No error handling for dispatch failures
  3. Potential undefined access even with optional chaining

Consider this safer implementation:

  dispatch(options: Agent.DispatchOptions, handler: Dispatcher.DispatchHandler): boolean {
-   const opaque = this.#opaqueLocalStorage?.getStore();
-   if (opaque) {
-     (handler as any).opaque = opaque;
-   }
-   return super.dispatch(options, handler);
+   try {
+     const opaque = this.#opaqueLocalStorage?.getStore();
+     if (opaque) {
+       // Use a type intersection instead of 'any'
+       (handler as Dispatcher.DispatchHandler & { opaque: FetchOpaque }).opaque = opaque;
+     }
+     return super.dispatch(options, handler);
+   } catch (error) {
+     // Log error or handle it appropriately
+     throw new Error(`Failed to dispatch request: ${error.message}`);
+   }
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
dispatch(options: Agent.DispatchOptions, handler: Dispatcher.DispatchHandler): boolean {
const opaque = this.#opaqueLocalStorage?.getStore();
if (opaque) {
(handler as any).opaque = opaque;
}
return super.dispatch(options, handler);
}
dispatch(options: Agent.DispatchOptions, handler: Dispatcher.DispatchHandler): boolean {
try {
const opaque = this.#opaqueLocalStorage?.getStore();
if (opaque) {
// Use a type intersection instead of 'any'
(handler as Dispatcher.DispatchHandler & { opaque: FetchOpaque }).opaque = opaque;
}
return super.dispatch(options, handler);
} catch (error) {
// Log error or handle it appropriately
throw new Error(`Failed to dispatch request: ${error.message}`);
}
}

}
12 changes: 0 additions & 12 deletions src/FetchOpaqueInterceptor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// const { AsyncLocalStorage } = require('node:async_hooks');
import { AsyncLocalStorage } from 'node:async_hooks';
import symbols from './symbols.js';
import { Dispatcher } from 'undici';

// const RedirectHandler = require('../handler/redirect-handler')

Expand All @@ -28,14 +27,3 @@ export interface FetchOpaque {
export interface OpaqueInterceptorOptions {
opaqueLocalStorage: AsyncLocalStorage<FetchOpaque>;
}

export function fetchOpaqueInterceptor(opts: OpaqueInterceptorOptions) {
const opaqueLocalStorage = opts?.opaqueLocalStorage;
return (dispatch: Dispatcher['dispatch']): Dispatcher['dispatch'] => {
return function redirectInterceptor(opts: Dispatcher.DispatchOptions, handler: Dispatcher.DispatchHandler) {
const opaque = opaqueLocalStorage?.getStore();
(handler as any).opaque = opaque;
return dispatch(opts, handler);
};
};
}
5 changes: 3 additions & 2 deletions src/HttpAgent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@ import {
Dispatcher,
buildConnector,
} from 'undici';
import { BaseAgent, BaseAgentOptions } from './BaseAgent.js';

export type CheckAddressFunction = (ip: string, family: number | string, hostname: string) => boolean;

export interface HttpAgentOptions extends Agent.Options {
export interface HttpAgentOptions extends BaseAgentOptions {
lookup?: LookupFunction;
checkAddress?: CheckAddressFunction;
connect?: buildConnector.BuildOptions,
Expand All @@ -31,7 +32,7 @@ class IllegalAddressError extends Error {
}
}

export class HttpAgent extends Agent {
export class HttpAgent extends BaseAgent {
#checkAddress?: CheckAddressFunction;

constructor(options: HttpAgentOptions) {
Expand Down
25 changes: 10 additions & 15 deletions src/fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
Agent,
getGlobalDispatcher,
Pool,
Dispatcher,
} from 'undici';
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
Expand Down Expand Up @@ -35,9 +36,10 @@ import {
HttpMethod,
RequestMeta,
} from './Request.js';
import { FetchOpaque, fetchOpaqueInterceptor } from './FetchOpaqueInterceptor.js';
import { FetchOpaque } from './FetchOpaqueInterceptor.js';
import { RawResponseWithMeta, SocketInfo } from './Response.js';
import { IncomingHttpHeaders } from './IncomingHttpHeaders.js';
import { BaseAgent, BaseAgentOptions } from './BaseAgent.js';

export interface UrllibRequestInit extends RequestInit {
// default is true
Expand All @@ -56,7 +58,7 @@ export type FetchResponseDiagnosticsMessage = {
};

export class FetchFactory {
static #dispatcher: Agent;
static #dispatcher: Dispatcher.ComposedDispatcher;
static #opaqueLocalStorage = new AsyncLocalStorage<FetchOpaque>();

static getDispatcher() {
Expand All @@ -68,17 +70,10 @@ export class FetchFactory {
}

static setClientOptions(clientOptions: ClientOptions) {
let dispatcherOption: Agent.Options = {
interceptors: {
Agent: [
fetchOpaqueInterceptor({
opaqueLocalStorage: FetchFactory.#opaqueLocalStorage,
}),
],
Client: [],
},
let dispatcherOption: BaseAgentOptions = {
opaqueLocalStorage: FetchFactory.#opaqueLocalStorage,
};
let dispatcherClazz: new (options: Agent.Options) => Agent = Agent;
let dispatcherClazz: new (options: BaseAgentOptions) => BaseAgent = BaseAgent;
if (clientOptions?.lookup || clientOptions?.checkAddress) {
dispatcherOption = {
...dispatcherOption,
Expand All @@ -87,21 +82,21 @@ export class FetchFactory {
connect: clientOptions.connect,
allowH2: clientOptions.allowH2,
} as HttpAgentOptions;
dispatcherClazz = HttpAgent as unknown as new (options: Agent.Options) => Agent;
dispatcherClazz = HttpAgent as unknown as new (options: BaseAgentOptions) => BaseAgent;
} else if (clientOptions?.connect) {
dispatcherOption = {
...dispatcherOption,
connect: clientOptions.connect,
allowH2: clientOptions.allowH2,
} as HttpAgentOptions;
dispatcherClazz = Agent;
dispatcherClazz = BaseAgent;
} else if (clientOptions?.allowH2) {
// Support HTTP2
dispatcherOption = {
...dispatcherOption,
allowH2: clientOptions.allowH2,
} as HttpAgentOptions;
dispatcherClazz = Agent;
dispatcherClazz = BaseAgent;
}
FetchFactory.#dispatcher = new dispatcherClazz(dispatcherOption);
initDiagnosticsChannel();
Expand Down
1 change: 1 addition & 0 deletions test/fetch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
assert(requestDiagnosticsMessage!.request);
assert(responseDiagnosticsMessage!.request);
assert(responseDiagnosticsMessage!.response);
assert.equal(responseDiagnosticsMessage!.response.socket.localAddress, '127.0.0.1');

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (macos-latest, 18)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (macos-latest, 18.19.0)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (macos-latest, 20)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (macos-latest, 22)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (macos-latest, 23)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:

'::1' !== '127.0.0.1'


Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (ubuntu-latest, 18)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (ubuntu-latest, 18.19.0)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (ubuntu-latest, 20)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (ubuntu-latest, 22)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (ubuntu-latest, 23)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:

'::1' !== '127.0.0.1'


Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (windows-latest, 18)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (windows-latest, 18.19.0)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (windows-latest, 20)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (windows-latest, 22)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:
+ actual - expected

+ '::1'
- '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12

Check failure on line 49 in test/fetch.test.ts

View workflow job for this annotation

GitHub Actions / Node.js / Test (windows-latest, 23)

test/fetch.test.ts > fetch.test.ts > fetch should work 

AssertionError: Expected values to be strictly equal:

'::1' !== '127.0.0.1'


Expected: "127.0.0.1"
Received: "::1"

 ❯ test/fetch.test.ts:49:12
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix Test Assertion to Support IPv6 Loopback Address

The test is failing because the localAddress is '::1' (IPv6 loopback) instead of '127.0.0.1' (IPv4 loopback). This can happen in environments where IPv6 is preferred. To make the test more robust, adjust the assertion to accept both IPv4 and IPv6 loopback addresses.

Apply this diff to update the assertion:

assert.equal(responseDiagnosticsMessage!.response.socket.localAddress, '127.0.0.1');
+ assert(['127.0.0.1', '::1'].includes(responseDiagnosticsMessage!.response.socket.localAddress));

Alternatively, use a regular expression to match any loopback address:

+ assert(/^(127\.0\.0\.1|::1)$/.test(responseDiagnosticsMessage!.response.socket.localAddress));

This adjustment ensures the test passes regardless of whether the environment uses IPv4 or IPv6 for loopback addresses.

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 23)

[failure] 49-49: test/fetch.test.ts > fetch.test.ts > fetch should work
AssertionError: Expected values to be strictly equal:

'::1' !== '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

❯ test/fetch.test.ts:49:12

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)

[failure] 49-49: test/fetch.test.ts > fetch.test.ts > fetch should work
AssertionError: Expected values to be strictly equal:

  • actual - expected

  • '::1'

  • '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

❯ test/fetch.test.ts:49:12

🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)

[failure] 49-49: test/fetch.test.ts > fetch.test.ts > fetch should work
AssertionError: Expected values to be strictly equal:

  • actual - expected

  • '::1'

  • '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

❯ test/fetch.test.ts:49:12

🪛 GitHub Check: Node.js / Test (macos-latest, 22)

[failure] 49-49: test/fetch.test.ts > fetch.test.ts > fetch should work
AssertionError: Expected values to be strictly equal:

  • actual - expected

  • '::1'

  • '127.0.0.1'

Expected: "127.0.0.1"
Received: "::1"

❯ test/fetch.test.ts:49:12


assert(fetchDiagnosticsMessage!.fetch);
assert(fetchResponseDiagnosticsMessage!.fetch);
Expand Down
Loading