fix(utils): Handle when requests get aborted in fetch instrumentation (…

…#13202) Co-authored-by: Charly Gomez <charly.gomez@sentry.io> Co-authored-by: Luca Forstner <luca.forstner@sentry.io>
getsentry · Aug 5, 2024 · 957324e · 957324e
1 parent 9289200
commit 957324e
Show file tree

Hide file tree

Showing 7 changed files with 169 additions and 36 deletions.
diff --git a/...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js b/...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/init.js
@@ -0,0 +1,8 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  tracesSampleRate: 1,
+});
diff --git a/...ser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js b/...ser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/subject.js
@@ -0,0 +1,36 @@
+let controller;
+
+const startFetch = e => {
+  controller = new AbortController();
+  const { signal } = controller;
+
+  Sentry.startSpan(
+    {
+      name: 'with-abort-controller',
+      forceTransaction: true,
+    },
+    async () => {
+      await fetch('http://localhost:7654/foo', { signal })
+        .then(response => response.json())
+        .then(data => {
+          console.log('Fetch succeeded:', data);
+        })
+        .catch(err => {
+          if (err.name === 'AbortError') {
+            console.log('Fetch aborted');
+          } else {
+            console.error('Fetch error:', err);
+          }
+        });
+    },
+  );
+};
+
+const abortFetch = e => {
+  if (controller) {
+    controller.abort();
+  }
+};
+
+document.querySelector('[data-test-id=start-button]').addEventListener('click', startFetch);
+document.querySelector('[data-test-id=abort-button]').addEventListener('click', abortFetch);
diff --git a/...-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html b/...-integration-tests/suites/integrations/httpclient/fetch/withAbortController/template.html
@@ -0,0 +1,10 @@
+<!doctype html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+  </head>
+  <body>
+    <button data-test-id="start-button">Start fetch</button>
+    <button data-test-id="abort-button">Abort fetch</button>
+  </body>
+</html>
diff --git a/...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts b/...rowser-integration-tests/suites/integrations/httpclient/fetch/withAbortController/test.ts
@@ -0,0 +1,41 @@
+import { expect } from '@playwright/test';
+import type { Event as SentryEvent } from '@sentry/types';
+import { sentryTest } from '../../../../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest, shouldSkipTracingTest } from '../../../../../utils/helpers';
+
+sentryTest('should handle aborted fetch calls', async ({ getLocalTestPath, page }) => {
+  if (shouldSkipTracingTest()) {
+    sentryTest.skip();
+  }
+
+  const url = await getLocalTestPath({ testDir: __dirname });
+
+  await page.route('**/foo', async () => {
+    // never fulfil this route because we abort the request as part of the test
+  });
+
+  const transactionEventPromise = getFirstSentryEnvelopeRequest<SentryEvent>(page);
+
+  const hasAbortedFetchPromise = new Promise<void>(resolve => {
+    page.on('console', msg => {
+      if (msg.type() === 'log' && msg.text() === 'Fetch aborted') {
+        resolve();
+      }
+    });
+  });
+
+  await page.goto(url);
+
+  await page.locator('[data-test-id=start-button]').click();
+  await page.locator('[data-test-id=abort-button]').click();
+
+  const transactionEvent = await transactionEventPromise;
+
+  // assert that fetch calls do not return undefined
+  const fetchBreadcrumbs = transactionEvent.breadcrumbs?.filter(
+    ({ category, data }) => category === 'fetch' && data === undefined,
+  );
+  expect(fetchBreadcrumbs).toHaveLength(0);
+
+  await expect(hasAbortedFetchPromise).resolves.toBeUndefined();
+});
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
@@ -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;
@@ -42,6 +49,9 @@ const SSE = () => {
       <button id="fetch-timeout-button" onClick={() => fetchSSE({ timeout: true })}>
         Fetch timeout SSE
       </button>
+      <button id="fetch-sse-abort" onClick={() => fetchSSE({ timeout: false, abort: true })}>
+        Fetch SSE with error
+      </button>
     </>
   );
 };

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
@@ -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
@@ -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<void> {
   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<void> {
   // clone response for awaiting stream
-  let clonedResponseForResolving: Response | undefined;
+  let clonedResponseForResolving: Response;
   try {
     clonedResponseForResolving = response.clone();
-  } catch (e) {
-    // noop
+  } catch {
+    return;
   }
 
   await resolveResponse(clonedResponseForResolving, () => {