Skip to content

fix: default keepalive=false for fetch #1016

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.

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

Merged
merged 1 commit into from
Oct 10, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/sixty-scissors-bake.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@smithy/fetch-http-handler": patch
---

set keepalive default to false in fetch handler
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe(FetchHttpHandler.name, () => {
const requestArgs = winReqSpy.calls.argsFor(0);
expect(requestArgs[0]).toEqual(expectedUrl);
expect(requestArgs[1].method).toEqual(mockHttpRequest.method);
expect(requestArgs[1].keepalive).toEqual(true);
expect(requestArgs[1].keepalive).toEqual(false);
});

for (const method of ["GET", "HEAD"]) {
Expand All @@ -45,15 +45,15 @@ describe(FetchHttpHandler.name, () => {
});
}

it(`sets keepalive to false if explicitly requested`, async () => {
const fetchHttpHandler = new FetchHttpHandler({ keepAlive: false });
it(`sets keepalive to true if explicitly requested`, async () => {
const fetchHttpHandler = new FetchHttpHandler({ keepAlive: true });
const winReqSpy = spyOn(window, "Request");

const mockHttpRequest = getMockHttpRequest({});
await fetchHttpHandler.handle(mockHttpRequest);

const requestArgs = winReqSpy.calls.argsFor(0);
expect(requestArgs[1].keepalive).toEqual(false);
expect(requestArgs[1].keepalive).toEqual(true);
});

it(`builds querystring if provided`, async () => {
Expand Down
8 changes: 4 additions & 4 deletions packages/fetch-http-handler/src/fetch-http-handler.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ describe(FetchHttpHandler.name, () => {
});

describe("keepalive", () => {
it("will pass keepalive as true by default to request if supported", async () => {
it("will pass keepalive as false by default to request if supported", async () => {
const mockResponse = {
headers: {
entries: jest.fn().mockReturnValue([
Expand All @@ -379,7 +379,7 @@ describe(FetchHttpHandler.name, () => {
keepAliveSupport.supported = true;
await fetchHttpHandler.handle({} as any, {});

expect(mockRequest.mock.calls[0][1].keepalive).toBe(true);
expect(mockRequest.mock.calls[0][1].keepalive).toBe(false);
});

it("will pass keepalive to request if supported", async () => {
Expand All @@ -395,12 +395,12 @@ describe(FetchHttpHandler.name, () => {
const mockFetch = jest.fn().mockResolvedValue(mockResponse);
(global as any).fetch = mockFetch;

const fetchHttpHandler = new FetchHttpHandler({ keepAlive: false });
const fetchHttpHandler = new FetchHttpHandler({ keepAlive: true });

keepAliveSupport.supported = true;
await fetchHttpHandler.handle({} as any, {});

expect(mockRequest.mock.calls[0][1].keepalive).toBe(false);
expect(mockRequest.mock.calls[0][1].keepalive).toBe(true);
});

it("will not have keepalive property in request if not supported", async () => {
Expand Down
10 changes: 8 additions & 2 deletions packages/fetch-http-handler/src/fetch-http-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,13 @@ export interface FetchHttpHandlerOptions {
requestTimeout?: number;

/**
* Whether to allow the request to outlive the page. Default value is true
* Whether to allow the request to outlive the page. Default value is false.
*
* There may be limitations to the payload size, number of concurrent requests,
* request duration etc. when using keepalive in browsers.
*
* These may change over time, so look for up to date information about
* these limitations before enabling keepalive.
*/
keepAlive?: boolean;
}
Expand Down Expand Up @@ -59,7 +65,7 @@ export class FetchHttpHandler implements HttpHandler<FetchHttpHandlerConfig> {
this.config = await this.configProvider;
}
const requestTimeoutInMs = this.config!.requestTimeout;
const keepAlive = this.config!.keepAlive ?? true;
const keepAlive = this.config!.keepAlive === true;

// if the request was already aborted, prevent doing extra work
if (abortSignal?.aborted) {
Expand Down