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.

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(browser): Ensure keepalive flag is correctly set for parallel requests #7553

Merged
merged 3 commits into from
Mar 21, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 18 additions & 8 deletions packages/browser/src/transports/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,12 @@ export function makeFetchTransport(
options: BrowserTransportOptions,
nativeFetch: FetchImpl = getNativeFetchImplementation(),
): Transport {
let pendingBodySize = 0;

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

const requestOptions: RequestInit = {
body: request.body,
method: 'POST',
Expand All @@ -28,20 +33,25 @@ export function makeFetchTransport(
// - 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,
// Note that the limit is for all pending requests, not per request, so we need to check this based on all current requests.
keepalive: pendingBodySize <= 65536,
...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;
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;
return rejectedSyncPromise(e);
}
}
Expand Down
52 changes: 52 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 * 1024) }] as EventItem],
);

class Headers {
headers: { [key: string]: string } = {};
get(key: string) {
Expand Down Expand Up @@ -107,4 +112,51 @@ 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 }));
}

// Limit resets when requests have resolved
const promises2 = [];
for (let i = 0; i < 10; i++) {
promises2.push(transport.send(LARGE_ERROR_ENVELOPE));
}

await Promise.all(promises2);

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