getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.99k stars 1.57k forks source link

Fetch resolves before response has been received; response is undefined #13187

Closed andreasphil closed 3 months ago

andreasphil commented 3 months ago

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/vue

SDK Version

8.22.0

Framework Version

Vue 3.4.35

Link to Sentry event

No response

Reproduction Example/SDK Setup

Unfortunately I don't have a reproduction, but this is our setup:

(Note that I debugged useFetch and it seems to not be a useFetch issue, see Steps to Reproduce below)

Steps to Reproduce

Summary: for some long-running (a few seconds, but <10s), fetch resolves before the actual request has finished. This leads to the fetch response being undefined. Anything accessing properties of the response then crashes. Reverting to @sentry/vue v8.20.0 fixes the issue.

Example:

fetch("some-long-running-request").then((response) => {
  // Now response is sometimes undefined, so the following would lead to a TypeError:
  if (response.status === 400) {
    // ...
  }
})

Looking at the network dev tools and server logs, I can verify that the request is fulfilled successfully. However it seems that fetch resolves before the request has finished, with undefined as the value for the response.

Possibly related:

Expected Result

Fetch should not resolve before the request is finished, and response should not be undefined.

Actual Result

Fetch resolves before the request has finished, response is undefined, and accessing properties of the response causes errors.

chargome commented 3 months ago

Hey @andreasphil thanks for reaching out. The PR you mentioned should not affect your original fetch call, as we just try to clone the response and resolve it in parallel.

Does the endpoint you are calling make use of SSE?

Anyway, I'll try to verify your issue and get back to you.

andreasphil commented 3 months ago

@chargome Thanks for the quick reply!

Yes, it might be unrelated. I'm not familiar with the Sentry code base, just did some quick scanning of the changelogs since v8.20 to see if anything catches my attention.

We're not using SSE.

Thanks for looking into it and let me know if you need more information.

esetnik commented 3 months ago

We have been hit by the same issue and are rolling back to 8.20.0. It looks to be related to https://github.com/getsentry/sentry-javascript/pull/12723

chargome commented 3 months ago

@esetnik are you also using the @sentry/vue SDK?

esetnik commented 3 months ago

@esetnik are you also using the @sentry/vue SDK?

No we are using @sentry/react

chargome commented 3 months ago

@andreasphil I couldn't reproduce this issue with a long running browser fetch request from vue (15s) - everything still worked with 8.22. Will try to see if useFetch has any impact. Do you have any details on how the request is handled on the server side?

@esetnik Could you also provide more details on your setup, maybe we can pin down the issue faster this way.

A small reproducible example would be really helpful for fixing this issue.

andreasphil commented 3 months ago

@chargome On the server side we're using a pretty standard Java Spring Boot REST API. This is one of the controllers for the requests that were starting to reliably cause issues after the SDK update.

FWIW all our code is public in the repo I linked (frontend + backend). It's not a small codebase though so I don't expect you to read all of it, but maybe it still helps.

Unfortunately I'm not sure if I will find the time to create a reproducible example but I'll see what I can do next week.

chargome commented 3 months ago

@andreasphil thanks, let's see if we can reproduce it somehow.

esetnik commented 3 months ago

I believe it's related to aborted fetch requests. We use an AbortController to abort the requests and seem to be seeing this only on endpoints where we actually frequently abort fetch requests.

AbhiPrasad commented 3 months ago

@esetnik great suggestion on the aborted request - that seems to be it

opened https://github.com/getsentry/sentry-javascript/pull/13202 to fix this, we'll get a release out on monday after I add (several) e2e tests 😄

andreasphil commented 3 months ago

@esetnik good catch, I was about to say something similar. I did some debugging with our app including the Sentry SDK, here are some findings:

This is where I'm going to stop for today but hope that helps. And again, thanks for looking into it!

AbhiPrasad commented 3 months ago

thanks for validating @andreasphil

and to everyone who runs into this, sorry for the trouble, we're on this to fix! Appreciate all the help.

lforst commented 3 months ago

Hi all, we just released version 8.23.0 of the SDK which includes a fix for this bug. Feel free to try it out and let us know if it resolved the issue for you. Thanks!

andreasphil commented 3 months ago

@lforst I can confirm the issue is fixed for us. Thank you!