Skip to content

Commit

Permalink
fix(browser): Ensure keepalive flag is correctly set for parallel req…
Browse files Browse the repository at this point in the history
…uests (#7553)

We noticed that sometimes request would remain in a seemingly pending state.
After some investigation, we found out that the limit of 64kb for keepalive-enabled fetch requests is not per request but for all parallel requests running at the same time.

This fixes this by keeping track of how large the pending body sizes are, plus the # of pending requests, and setting keepalive accordingly.
  • Loading branch information
mydea authored Mar 21, 2023
1 parent 5c5ac2c commit 7b9198f
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 11 deletions.
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 }));
}
});
});

0 comments on commit 7b9198f

Please sign in to comment.