Skip to content

Commit 79ee8af

Browse files
committed
fix: skip implicit timeout for responseType: "stream", default to "GET" to avoid undefined request method, streaming timeout kills long-running streams
1 parent ef3a638 commit 79ee8af

3 files changed

Lines changed: 128 additions & 12 deletions

File tree

client.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1066,7 +1066,7 @@ export class OpenFgaClient extends BaseAPI {
10661066
async executeStreamedApiRequest(
10671067
request: RequestBuilderParams,
10681068
options: ClientRequestOpts = {}
1069-
): PromiseResult<any> {
1069+
): Promise<any> {
10701070
return this.api.executeStreamedApiRequest(request, options);
10711071
}
10721072
}

common.ts

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -303,16 +303,19 @@ export async function attemptHttpRequest<B, R>(
303303
}
304304
}
305305

306-
const timeoutMs = request.timeout ?? httpClient.defaultTimeout ?? 10000;
306+
const timeoutMs = request.responseType === "stream" && request.timeout == null ?
307+
undefined : request.timeout ?? httpClient.defaultTimeout ?? 10000;
307308
let signal: AbortSignal | undefined;
308-
if (typeof AbortSignal !== "undefined" && typeof (AbortSignal as any).timeout === "function") {
309-
// Use native AbortSignal.timeout when available.
310-
signal = (AbortSignal as any).timeout(timeoutMs);
311-
} else if (typeof AbortController !== "undefined") {
312-
// Fallback for environments without AbortSignal.timeout.
313-
const controller = new AbortController();
314-
setTimeout(() => controller.abort(), timeoutMs);
315-
signal = controller.signal;
309+
if (timeoutMs !== undefined) {
310+
if (typeof AbortSignal !== "undefined" && typeof (AbortSignal as any).timeout === "function") {
311+
// Use native AbortSignal.timeout when available.
312+
signal = (AbortSignal as any).timeout(timeoutMs);
313+
} else if (typeof AbortController !== "undefined") {
314+
// Fallback for environments without AbortSignal.timeout.
315+
const controller = new AbortController();
316+
setTimeout(() => controller.abort(), timeoutMs);
317+
signal = controller.signal;
318+
}
316319
}
317320
const fetchInit: RequestInit = {
318321
method: request.method || "GET",
@@ -378,7 +381,7 @@ export async function attemptHttpRequest<B, R>(
378381
headers: responseHeaders,
379382
data: responseData,
380383
requestUrl: request.url,
381-
requestMethod: request.method,
384+
requestMethod: request.method ?? "GET",
382385
requestData: request.data,
383386
};
384387

@@ -469,7 +472,7 @@ export async function attemptHttpRequest<B, R>(
469472

470473
const errCtx: HttpErrorContext = {
471474
requestUrl: request.url,
472-
requestMethod: request.method,
475+
requestMethod: request.method ?? "GET",
473476
requestData: request.data,
474477
};
475478

tests/fetch-http-client.test.ts

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,83 @@ describe("fetch-based HTTP client", () => {
244244
});
245245
});
246246

247+
describe("streaming timeout", () => {
248+
it("should not attach an abort signal for stream requests without explicit timeout", async () => {
249+
let capturedSignal: AbortSignal | null | undefined;
250+
const stream = new ReadableStream({
251+
start(controller) {
252+
controller.enqueue(new TextEncoder().encode("{\"result\":\"ok\"}\n"));
253+
controller.close();
254+
},
255+
});
256+
257+
const client = mockHttpClient(
258+
async (_url, init) => {
259+
capturedSignal = init?.signal;
260+
return new Response(stream, { status: 200, headers: { "content-type": "application/json" } });
261+
},
262+
{ defaultTimeout: 10000 }
263+
);
264+
265+
await attemptHttpRequest(
266+
{ url: `${SdkConstants.TestApiUrl}/stream`, method: "POST", headers: {}, responseType: "stream" },
267+
{ maxRetry: 0, minWaitInMs: 100 },
268+
client
269+
);
270+
271+
// Stream requests without explicit timeout should NOT have a signal
272+
expect(capturedSignal).toBeUndefined();
273+
});
274+
275+
it("should attach an abort signal for stream requests with explicit timeout", async () => {
276+
let capturedSignal: AbortSignal | null | undefined;
277+
const stream = new ReadableStream({
278+
start(controller) {
279+
controller.enqueue(new TextEncoder().encode("{\"result\":\"ok\"}\n"));
280+
controller.close();
281+
},
282+
});
283+
284+
const client = mockHttpClient(
285+
async (_url, init) => {
286+
capturedSignal = init?.signal;
287+
return new Response(stream, { status: 200, headers: { "content-type": "application/json" } });
288+
},
289+
{ defaultTimeout: 10000 }
290+
);
291+
292+
await attemptHttpRequest(
293+
{ url: `${SdkConstants.TestApiUrl}/stream`, method: "POST", headers: {}, responseType: "stream", timeout: 30000 },
294+
{ maxRetry: 0, minWaitInMs: 100 },
295+
client
296+
);
297+
298+
// Stream requests WITH explicit timeout should have a signal
299+
expect(capturedSignal).toBeDefined();
300+
expect(capturedSignal).toBeInstanceOf(AbortSignal);
301+
});
302+
303+
it("should still attach an abort signal for non-stream requests", async () => {
304+
let capturedSignal: AbortSignal | null | undefined;
305+
const client = mockHttpClient(
306+
async (_url, init) => {
307+
capturedSignal = init?.signal;
308+
return mockResponse(200, {});
309+
},
310+
{ defaultTimeout: 10000 }
311+
);
312+
313+
await attemptHttpRequest(
314+
{ url: `${SdkConstants.TestApiUrl}/check`, method: "POST", headers: {} },
315+
{ maxRetry: 0, minWaitInMs: 100 },
316+
client
317+
);
318+
319+
expect(capturedSignal).toBeDefined();
320+
expect(capturedSignal).toBeInstanceOf(AbortSignal);
321+
});
322+
});
323+
247324
describe("timeout", () => {
248325
it("should pass AbortSignal.timeout to fetch", async () => {
249326
let capturedSignal: AbortSignal | undefined;
@@ -478,6 +555,42 @@ describe("fetch-based HTTP client", () => {
478555
expect(callCount).toBe(1);
479556
});
480557

558+
it("should default requestMethod to GET in error context when method is omitted", async () => {
559+
const client = mockHttpClient(async () =>
560+
mockResponse(400, { code: "validation_error", message: "bad" }, { statusText: "Bad Request" })
561+
);
562+
563+
try {
564+
await attemptHttpRequest(
565+
{ url: `${SdkConstants.TestApiUrl}/stores/s1/check`, headers: {} },
566+
{ maxRetry: 0, minWaitInMs: 100 },
567+
client
568+
);
569+
fail("should have thrown");
570+
} catch (err: any) {
571+
expect(err).toBeInstanceOf(FgaApiValidationError);
572+
expect(err.method).toBe("GET");
573+
}
574+
});
575+
576+
it("should default requestMethod to GET in network error context when method is omitted", async () => {
577+
const client = mockHttpClient(async () => {
578+
throw new TypeError("Failed to fetch");
579+
});
580+
581+
try {
582+
await attemptHttpRequest(
583+
{ url: `${SdkConstants.TestApiUrl}/stores/s1/check`, headers: {} },
584+
{ maxRetry: 0, minWaitInMs: 1 },
585+
client
586+
);
587+
fail("should have thrown");
588+
} catch (err: any) {
589+
expect(err).toBeInstanceOf(FgaError);
590+
expect(err.message).toContain("GET");
591+
}
592+
});
593+
481594
it("should include method, URL, and status in generic FgaApiError message for unmapped status", async () => {
482595
const client = mockHttpClient(async () =>
483596
mockResponse(501, {}, { statusText: "Not Implemented" })

0 commit comments

Comments
 (0)