Skip to content

fix(browser): Ensure keepalive flag is correctly set for parallel requests #7553

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 3 commits into from
Mar 21, 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
37 changes: 26 additions & 11 deletions packages/browser/src/transports/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,14 @@ export function makeFetchTransport(
options: BrowserTransportOptions,
nativeFetch: FetchImpl = getNativeFetchImplementation(),
): Transport {
let pendingBodySize = 0;
let pendingCount = 0;

function makeRequest(request: TransportRequest): PromiseLike<TransportMakeRequestResponse> {
const requestSize = request.body.length;
pendingBodySize += requestSize;
pendingCount++;

const requestOptions: RequestInit = {
body: request.body,
method: 'POST',
Expand All @@ -25,23 +32,31 @@ export function makeFetchTransport(
// frequently sending events right before the user is switching pages (eg. whenfinishing navigation transactions).
// Gotchas:
// - `keepalive` isn't supported by Firefox
// - As per spec (https://fetch.spec.whatwg.org/#http-network-or-cache-fetch), a request with `keepalive: true`
// and a content length of > 64 kibibytes returns a network error. We will therefore only activate the flag when
// we're below that limit.
keepalive: request.body.length <= 65536,
// - As per spec (https://fetch.spec.whatwg.org/#http-network-or-cache-fetch):
// If the sum of contentLength and inflightKeepaliveBytes is greater than 64 kibibytes, then return a network error.
// We will therefore only activate the flag when we're below that limit.
// There is also a limit of requests that can be open at the same time, so we also limit this to 15
// See https://github.com/getsentry/sentry-javascript/pull/7553 for details
keepalive: pendingBodySize <= 60_000 && pendingCount < 15,
...options.fetchOptions,
};

try {
return nativeFetch(options.url, requestOptions).then(response => ({
statusCode: response.status,
headers: {
'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'),
'retry-after': response.headers.get('Retry-After'),
},
}));
return nativeFetch(options.url, requestOptions).then(response => {
pendingBodySize -= requestSize;
pendingCount--;
return {
statusCode: response.status,
headers: {
'x-sentry-rate-limits': response.headers.get('X-Sentry-Rate-Limits'),
'retry-after': response.headers.get('Retry-After'),
},
};
});
} catch (e) {
clearCachedFetchImplementation();
pendingBodySize -= requestSize;
pendingCount--;
return rejectedSyncPromise(e);
}
}
Expand Down
55 changes: 55 additions & 0 deletions packages/browser/test/unit/transports/fetch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ const ERROR_ENVELOPE = createEnvelope<EventEnvelope>({ event_id: 'aa3ff046696b4b
[{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem,
]);

const LARGE_ERROR_ENVELOPE = createEnvelope<EventEnvelope>(
{ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' },
[[{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', message: 'x'.repeat(10 * 900) }] as EventItem],
);

class Headers {
headers: { [key: string]: string } = {};
get(key: string) {
Expand Down Expand Up @@ -107,4 +112,54 @@ describe('NewFetchTransport', () => {
await expect(() => transport.send(ERROR_ENVELOPE)).not.toThrow();
expect(mockFetch).toHaveBeenCalledTimes(1);
});

it('correctly sets keepalive flag', async () => {
const mockFetch = jest.fn(() =>
Promise.resolve({
headers: new Headers(),
status: 200,
text: () => Promise.resolve({}),
}),
) as unknown as FetchImpl;

const REQUEST_OPTIONS: RequestInit = {
referrerPolicy: 'strict-origin',
referrer: 'http://example.org',
};

const transport = makeFetchTransport(
{ ...DEFAULT_FETCH_TRANSPORT_OPTIONS, fetchOptions: REQUEST_OPTIONS },
mockFetch,
);

const promises: PromiseLike<unknown>[] = [];
for (let i = 0; i < 30; i++) {
promises.push(transport.send(LARGE_ERROR_ENVELOPE));
}

await Promise.all(promises);

for (let i = 1; i <= 30; i++) {
// After 7 requests, we hit the total limit of >64kb of size
// Starting there, keepalive should be false
const keepalive = i < 7;
expect(mockFetch).toHaveBeenNthCalledWith(i, expect.any(String), expect.objectContaining({ keepalive }));
}

(mockFetch as jest.Mock<unknown>).mockClear();

// Limit resets when requests have resolved
// Now try based on # of pending requests
const promises2 = [];
for (let i = 0; i < 20; i++) {
promises2.push(transport.send(ERROR_ENVELOPE));
}

await Promise.all(promises2);

for (let i = 1; i <= 20; i++) {
const keepalive = i < 15;
expect(mockFetch).toHaveBeenNthCalledWith(i, expect.any(String), expect.objectContaining({ keepalive }));
}
});
});