From 93c78b6436d8d50375951128a51fc06770ad4272 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 21 Mar 2023 15:14:09 +0100 Subject: [PATCH 1/3] fix: Ensure keepalive flag is correctly set for parallel requests --- packages/browser/src/transports/fetch.ts | 26 +++++++--- .../test/unit/transports/fetch.test.ts | 52 +++++++++++++++++++ 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts index 83b75c82ba39..6976a1129ab2 100644 --- a/packages/browser/src/transports/fetch.ts +++ b/packages/browser/src/transports/fetch.ts @@ -13,7 +13,12 @@ export function makeFetchTransport( options: BrowserTransportOptions, nativeFetch: FetchImpl = getNativeFetchImplementation(), ): Transport { + let pendingBodySize = 0; + function makeRequest(request: TransportRequest): PromiseLike { + const requestSize = request.body.length; + pendingBodySize += requestSize; + const requestOptions: RequestInit = { body: request.body, method: 'POST', @@ -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); } } diff --git a/packages/browser/test/unit/transports/fetch.test.ts b/packages/browser/test/unit/transports/fetch.test.ts index 53ed5c34e21b..abce3a995483 100644 --- a/packages/browser/test/unit/transports/fetch.test.ts +++ b/packages/browser/test/unit/transports/fetch.test.ts @@ -16,6 +16,11 @@ const ERROR_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4b [{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem, ]); +const LARGE_ERROR_ENVELOPE = createEnvelope( + { 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) { @@ -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[] = []; + 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 })); + } + }); }); From 6027ed7e20656e2a82c1747cf75605878ad90452 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 21 Mar 2023 15:40:09 +0100 Subject: [PATCH 2/3] update doc --- packages/browser/src/transports/fetch.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts index 6976a1129ab2..d24f7868198e 100644 --- a/packages/browser/src/transports/fetch.ts +++ b/packages/browser/src/transports/fetch.ts @@ -30,10 +30,9 @@ 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. - // Note that the limit is for all pending requests, not per request, so we need to check this based on all current requests. + // - 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. keepalive: pendingBodySize <= 65536, ...options.fetchOptions, }; From 68d838bf5bad9722acfe772832e620ded111521e Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 21 Mar 2023 16:15:57 +0100 Subject: [PATCH 3/3] add count limit --- packages/browser/src/transports/fetch.ts | 8 +++++++- packages/browser/test/unit/transports/fetch.test.ts | 13 ++++++++----- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/browser/src/transports/fetch.ts b/packages/browser/src/transports/fetch.ts index d24f7868198e..e996b6f8277a 100644 --- a/packages/browser/src/transports/fetch.ts +++ b/packages/browser/src/transports/fetch.ts @@ -14,10 +14,12 @@ export function makeFetchTransport( nativeFetch: FetchImpl = getNativeFetchImplementation(), ): Transport { let pendingBodySize = 0; + let pendingCount = 0; function makeRequest(request: TransportRequest): PromiseLike { const requestSize = request.body.length; pendingBodySize += requestSize; + pendingCount++; const requestOptions: RequestInit = { body: request.body, @@ -33,13 +35,16 @@ export function makeFetchTransport( // - 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. - keepalive: pendingBodySize <= 65536, + // 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 => { pendingBodySize -= requestSize; + pendingCount--; return { statusCode: response.status, headers: { @@ -51,6 +56,7 @@ export function makeFetchTransport( } catch (e) { clearCachedFetchImplementation(); pendingBodySize -= requestSize; + pendingCount--; return rejectedSyncPromise(e); } } diff --git a/packages/browser/test/unit/transports/fetch.test.ts b/packages/browser/test/unit/transports/fetch.test.ts index abce3a995483..e6b58eeaa110 100644 --- a/packages/browser/test/unit/transports/fetch.test.ts +++ b/packages/browser/test/unit/transports/fetch.test.ts @@ -18,7 +18,7 @@ const ERROR_ENVELOPE = createEnvelope({ event_id: 'aa3ff046696b4b const LARGE_ERROR_ENVELOPE = createEnvelope( { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' }, - [[{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', message: 'x'.repeat(10 * 1024) }] as EventItem], + [[{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', message: 'x'.repeat(10 * 900) }] as EventItem], ); class Headers { @@ -146,16 +146,19 @@ describe('NewFetchTransport', () => { expect(mockFetch).toHaveBeenNthCalledWith(i, expect.any(String), expect.objectContaining({ keepalive })); } + (mockFetch as jest.Mock).mockClear(); + // Limit resets when requests have resolved + // Now try based on # of pending requests const promises2 = []; - for (let i = 0; i < 10; i++) { - promises2.push(transport.send(LARGE_ERROR_ENVELOPE)); + for (let i = 0; i < 20; i++) { + promises2.push(transport.send(ERROR_ENVELOPE)); } await Promise.all(promises2); - for (let i = 1; i <= 10; i++) { - const keepalive = i < 7; + for (let i = 1; i <= 20; i++) { + const keepalive = i < 15; expect(mockFetch).toHaveBeenNthCalledWith(i, expect.any(String), expect.objectContaining({ keepalive })); } });