From f260a3670a6e21c86170bb208117aa197d97129d Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Fri, 2 Aug 2024 12:51:47 -0400 Subject: [PATCH] fix(utils): Handle when requests get aborted --- .../react-router-6/src/pages/SSE.tsx | 15 ++++- .../react-router-6/tests/sse.test.ts | 37 ++++++++++++ packages/utils/src/instrument/fetch.ts | 59 ++++++++----------- 3 files changed, 75 insertions(+), 36 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx b/dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx index 49e53b09cfa2..0c7fb9f8b54c 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx +++ b/dev-packages/e2e-tests/test-applications/react-router-6/src/pages/SSE.tsx @@ -2,17 +2,24 @@ import * as Sentry from '@sentry/react'; // biome-ignore lint/nursery/noUnusedImports: Need React import for JSX import * as React from 'react'; -const fetchSSE = async ({ timeout }: { timeout: boolean }) => { +const fetchSSE = async ({ timeout, abort = false }: { timeout: boolean, abort?: boolean }) => { Sentry.startSpanManual({ name: 'sse stream using fetch' }, async span => { + const controller = new AbortController(); + const res = await Sentry.startSpan({ name: 'sse fetch call' }, async () => { const endpoint = `http://localhost:8080/${timeout ? 'sse-timeout' : 'sse'}`; - return await fetch(endpoint); + + const signal = controller.signal; + return await fetch(endpoint, { signal }); }); const stream = res.body; const reader = stream?.getReader(); const readChunk = async () => { + if (abort) { + controller.abort(); + } const readRes = await reader?.read(); if (readRes?.done) { return; @@ -27,6 +34,7 @@ const fetchSSE = async ({ timeout }: { timeout: boolean }) => { await readChunk(); } catch (error) { console.error('Could not fetch sse', error); + (window as any)._FETCH_ABORT_ERROR = error; } span.end(); @@ -42,6 +50,9 @@ const SSE = () => { + ); }; diff --git a/dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts b/dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts index 5d4533726e36..92c06543c0b8 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-6/tests/sse.test.ts @@ -34,6 +34,43 @@ test('Waits for sse streaming when creating spans', async ({ page }) => { expect(resolveBodyDuration).toBe(2); }); +test('Waits for sse streaming when sse has been explicitly aborted', async ({ page }) => { + await page.goto('/sse'); + + const transactionPromise = waitForTransaction('react-router-6', async transactionEvent => { + return !!transactionEvent?.transaction && transactionEvent.contexts?.trace?.op === 'pageload'; + }); + + const fetchButton = page.locator('id=fetch-sse-abort'); + await fetchButton.click(); + + const rootSpan = await transactionPromise; + console.log(JSON.stringify(rootSpan, null, 2)); + const sseFetchCall = rootSpan.spans?.filter(span => span.description === 'sse fetch call')[0] as SpanJSON; + const httpGet = rootSpan.spans?.filter(span => span.description === 'GET http://localhost:8080/sse')[0] as SpanJSON; + + expect(sseFetchCall).toBeDefined(); + expect(httpGet).toBeDefined(); + + expect(sseFetchCall?.timestamp).toBeDefined(); + expect(sseFetchCall?.start_timestamp).toBeDefined(); + expect(httpGet?.timestamp).toBeDefined(); + expect(httpGet?.start_timestamp).toBeDefined(); + + // http headers get sent instantly from the server + const resolveDuration = Math.round((sseFetchCall.timestamp as number) - sseFetchCall.start_timestamp); + + // body streams after 0s because it has been aborted + const resolveBodyDuration = Math.round((httpGet.timestamp as number) - httpGet.start_timestamp); + + expect(resolveDuration).toBe(0); + expect(resolveBodyDuration).toBe(0); + + // validate abort eror was thrown by inspecting console + const consoleBreadcrumb = rootSpan.breadcrumbs?.find(breadcrumb => breadcrumb.category === 'console'); + expect(consoleBreadcrumb?.message).toBe('Could not fetch sse AbortError: BodyStreamBuffer was aborted'); +}); + test('Aborts when stream takes longer than 5s', async ({ page }) => { await page.goto('/sse'); diff --git a/packages/utils/src/instrument/fetch.ts b/packages/utils/src/instrument/fetch.ts index 0ea1a4ec8d9f..afa209c01929 100644 --- a/packages/utils/src/instrument/fetch.ts +++ b/packages/utils/src/instrument/fetch.ts @@ -80,47 +80,42 @@ function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNat if (onFetchResolved) { onFetchResolved(response); } else { - const finishedHandlerData: HandlerDataFetch = { + triggerHandlers('fetch', { ...handlerData, endTimestamp: timestampInSeconds() * 1000, response, - }; - triggerHandlers('fetch', finishedHandlerData); + }); } return response; }, (error: Error) => { - if (!onFetchResolved) { - const erroredHandlerData: HandlerDataFetch = { - ...handlerData, - endTimestamp: timestampInSeconds() * 1000, - error, - }; - - triggerHandlers('fetch', erroredHandlerData); - - if (isError(error) && error.stack === undefined) { - // NOTE: If you are a Sentry user, and you are seeing this stack frame, - // it means the error, that was caused by your fetch call did not - // have a stack trace, so the SDK backfilled the stack trace so - // you can see which fetch call failed. - error.stack = virtualStackTrace; - addNonEnumerableProperty(error, 'framesToPop', 1); - } + triggerHandlers('fetch', { + ...handlerData, + endTimestamp: timestampInSeconds() * 1000, + error, + }); + if (isError(error) && error.stack === undefined) { // NOTE: If you are a Sentry user, and you are seeing this stack frame, - // it means the sentry.javascript SDK caught an error invoking your application code. - // This is expected behavior and NOT indicative of a bug with sentry.javascript. - throw error; + // it means the error, that was caused by your fetch call did not + // have a stack trace, so the SDK backfilled the stack trace so + // you can see which fetch call failed. + error.stack = virtualStackTrace; + addNonEnumerableProperty(error, 'framesToPop', 1); } + + // NOTE: If you are a Sentry user, and you are seeing this stack frame, + // it means the sentry.javascript SDK caught an error invoking your application code. + // This is expected behavior and NOT indicative of a bug with sentry.javascript. + throw error; }, ); }; }); } -function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): void { +async function resolveResponse(res: Response | undefined, onFinishedResolving: () => void): Promise { if (res && res.body) { const responseReader = res.body.getReader(); @@ -146,25 +141,21 @@ function resolveResponse(res: Response | undefined, onFinishedResolving: () => v } } - responseReader + return responseReader .read() .then(consumeChunks) - .then(() => { - onFinishedResolving(); - }) - .catch(() => { - // noop - }); + .then(onFinishedResolving) + .catch(() => undefined); } } async function streamHandler(response: Response): Promise { // clone response for awaiting stream - let clonedResponseForResolving: Response | undefined; + let clonedResponseForResolving: Response; try { clonedResponseForResolving = response.clone(); - } catch (e) { - // noop + } catch { + return; } await resolveResponse(clonedResponseForResolving, () => {