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

[@sentry/nextjs] Errored API routes trigger "API resolved without sending a response" #3852

Closed
5 tasks done
adarnon opened this issue Jul 30, 2021 · 32 comments · Fixed by #4139 or #6070
Closed
5 tasks done

[@sentry/nextjs] Errored API routes trigger "API resolved without sending a response" #3852

adarnon opened this issue Jul 30, 2021 · 32 comments · Fixed by #4139 or #6070
Assignees

Comments

@adarnon
Copy link

adarnon commented Jul 30, 2021

Package + Version

  • @sentry/nextjs

Version:

6.10.0

Description

Describe your issue in detail, ideally, you have a reproducible demo that you can show.

The withSentry() API route wrapper monkeypatches res.end() function to await for flush before closing a request. While this is certainly useful for serverless environments, it could cause a false positive warning on deployments with a custom server (e.g., Express).

Since normal Next API route code does not await for res.end() to finish, the handler's promise returns before the response has ended properly. This causes Next to detect the API route as having finished without a response, even though the response is sent a few moments later. Here is an example:

imageedit_3_8075703349

A workaround is to wrap all API routes with another callback that always performs await res.end() before returning.

@etrepum
Copy link

etrepum commented Aug 8, 2021

We're getting these false positives as well. I haven't put in the time to look at testing this but I think that the only way to workaround this issue is in the wrapper, since there are transitive calls to res.end in express (e.g. res.send or res.json). I think that attaching a promise to the context would be sufficient, like this: https://github.com/getsentry/sentry-javascript/compare/master...etrepum:resolve-with-response-gh-3852?expand=1&w=1

@jordymeow
Copy link

It does this for all my requests as well, even though I do this:

return res.status(200).json({ success: true });

I hope this will be fixed :)

@KieraDOG
Copy link

Any plan on this one? I'm currently doing something like this to get rid of the message

import * as Sentry from '@sentry/nextjs';

// ISSUE: https://github.com/getsentry/sentry-javascript/issues/3852
// API resolved without sending a response for *, this may result in stalled requests.
const withSentry = (fn) => async (req, res) => {
  await Sentry.withSentry(fn)(req, res);
  await res.end();
};

export default withSentry;

@adikari
Copy link

adikari commented Sep 2, 2021

I am having the same issue in development. I have not deployed it to production to verify yet.

This gets logged in the server log:

API resolved without sending a response for /api/graphql, this may result in stalled requests.

And no error is logged in sentry.

If I have the barebone code from the example, it works fine. However, my actual code does a call to server and uses await to wait for the response. If there is error in server, then I don't get any report in sentry.

@kaykdm
Copy link

kaykdm commented Sep 14, 2021

You can set externalResolver to true in api route config since it is handled by sentry (external)
https://nextjs.org/docs/api-routes/api-middlewares#custom-config

@benjamintd
Copy link

benjamintd commented Sep 14, 2021

@kaykdm Thanks for this workaround! Is there a way to set a default config for all API routes ? From the docs it seems that there is no option to do this in next.config.js, which means this config has to be added to all API routes individually.

@KieraDOG the workaround in this comment did not work for me. This resulted in the response being ended before the actual content is sent, which means stalling requests (especially in a development environment).

@maccman
Copy link

maccman commented Sep 25, 2021

Why are you guys monkey patching .end() ?

https://github.com/getsentry/sentry-javascript/blob/master/packages/nextjs/src/utils/withSentry.ts#L22

Can you not just return a promise to Next?

@github-actions
Copy link
Contributor

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@adarnon
Copy link
Author

adarnon commented Oct 17, 2021

Please don't close - this is still an issue that should be resolved

@arclogos132
Copy link

Is this fixed?

lobsterkatie added a commit that referenced this issue Nov 10, 2021
…warning (#4139)

This prevents a false positive warning from nextjs which arises from the interaction of our API route wrapper `withSentry()` with nextjs's native API route handling.

In dev, nextjs checks that API route handlers return a response to the client before they resolve, and it throws a warning[1] if this hasn't happened. Meanwhile, in `withSentry()`, we wrap the `res.end()` method to ensure that events are flushed before the request/response lifecycle finishes.

As a result, there are cases where the handler resolves before the response is finished, while flushing is still in progress. This triggers the warning mentioned above, but it's a false alarm - the response may not have ended _yet_, but it will end. This suppresses the warning by temporarily marking the response finished, and then restoring it to unfinished before the original `res.end()` is called.

The fix here is simple, but the async-i-ness of it all leads to some complex interactions in sequencing between the SDK, nextjs, and Node itself. I've tried to lay it out as clearly as I can in comments.

Fixes #4007
Fixes #3852

[1] https://github.com/vercel/next.js/blob/e1464ae5a5061ae83ad015018d4afe41f91978b6/packages/next/server/api-utils.ts#L106-L118
@adikari
Copy link

adikari commented Nov 12, 2021

I have upgraded to latest version and I am still getting this error

image

@adarnon
Copy link
Author

adarnon commented Nov 12, 2021

@adikari This is a different error

@adikari
Copy link

adikari commented Nov 12, 2021

@adarnon my bad. I Mixed up different issues :)

lobsterkatie added a commit that referenced this issue Feb 8, 2022
…ue (#4516)

In #4139, a change was introduced in order to suppress a false positive warning thrown by nextjs, by setting `res.finished` to `true` just long enough for nextjs to check it and decide no warning was needed. In simple cases, it worked just fine, but in some set-ups the "just long enough for nextjs to check it and calm down" turned out to also be "just long enough for other things to check it and get mad."

This backs out that change, as it seems it's doing more harm than good. In order to address the original problem (documented in [1] and [2]), a new warning is added, explaining that the false positive is just that. Not as elegant as suppressing the message altogether, but it should tide us over until such time as we're able to try again with a different approach.

Fixes #4151.

[1] #3852
[2] #4007
@federico-moretti
Copy link

Why is this error closed? With the last version I still see the error, there is a warning but the problem is still there.

@Fedeorlandau
Copy link

Hi there, putting my comment here.

After #4516 I'm getting a massive amount of logs (I'm using next-auth + swr)

image

Thank you for your hard work :)

@lobsterkatie
Copy link
Member

lobsterkatie commented Mar 11, 2022

Why is this error closed?

Because it was fixed... in a way which turned out to break other things, and had to be reverted. I'll reopen.

As for finding a new solution, I'm open to ideas. We've been focusing on trying to ship our next major version, so I haven't been able to dive back into this at a deep level, but I will admit that even coming up with my turned-out-to-cause-other-problems hack took quite a bit of poking and prodding and digging through not only nextjs source code but node source code as well, and so I don't expect finding a new approach is going to be a trivial problem.

In the meantime, you can suppress next's warning yourself by using the "external resolver" option in your API route (docs here). I've also just pushed #4706, to allow our warning to be suppressed as well.

@lobsterkatie lobsterkatie reopened this Mar 11, 2022
@maxzaleski
Copy link

maxzaleski commented Apr 15, 2022

You can set externalResolver to true in api route config since it is handled by sentry (external) https://nextjs.org/docs/api-routes/api-middlewares#custom-config

I am running a tRPC backend through NextJS with a Sentry wrapper, and this is by far the nicest solution imo. It worked like a charm.

edit: grammar

@dgattey
Copy link

dgattey commented May 6, 2022

Setting externalResolver to true has no effect on this for me. Adding a wrapper around withSentry to await it, then response.end() doesn't work either. I don't see a resolution that works anywhere here with latest NextJS and latest Sentry.

This is my API code to test - very trivial

const handler = (request: NextApiRequest, response: NextApiResponse) => {
  response.status(200);
  response.end();
};

export default withSentry(handler);

EDIT: if I remove the withSentryConfig wrapper from my next.config.js, THEN the externalResolver has an effect. This is such a bad experience because of Sentry. Why is the withSentryConfig overwriting the option that I need to fix the Sentry-created error?

@smeubank smeubank added the Jira label May 19, 2022
@rafaelnogueira1
Copy link

You can set externalResolver to true in api route config since it is handled by sentry (external) https://nextjs.org/docs/api-routes/api-middlewares#custom-config

I use this config and the messages at the console stop but still does not log the errors in Sentry Dashboard. I change the withSentry hoc in my api for Sentry.captureException(error) and it works. Just need to create a personal hoc to avoid writing this every time.
I don't know if it's the best approach but for now it's all i got.

@leerobert
Copy link

We ever fixing this or what? Annoying to have logs spammed.

@leerobert
Copy link

Ok I made a quick little hack around until this issue is resolved... it's not the cleanest but at the end of each api/endpoint.ts file I put the following:

async function handler(req: NextApiRequest, res: NextApiResponse) {
....
}

export const config = {
  api: {
    externalResolver: true,
  },
};

export default withSentry(handler);

Annoying but works. next.config.js gets overwritten by withSentryConfig so you gotta do this to each route.

@rafaelnogueira1
Copy link

This is the way that is in the Sentry docs. But for me it doesn't show anything at Sentry dashboard :/.

@bsplosion
Copy link

bsplosion commented Aug 8, 2022

It seems like the SENTRY_IGNORE_API_RESOLUTION_ERROR environment variable is not being respected anymore either. This is especially ironic since we also added the externalResolver API prop on every handler, so we get no logging from NextJS but we get Sentry's warning that NextJS might log for every request. Is that variable still working for anyone?

Edit: I don't even see anything in the repo that'd be looking for such a variable, but maybe it's buried in the CLI itself? In my case it's being provided in next.config.js alongside other env variables such as SENTRY_URL, SENTRY_PROJECT, etc, and all of those are working fine.

@reuben-pct
Copy link

reuben-pct commented Aug 9, 2022

Same boat as @bsplosion, externalResolver workaround does silence the next.js warning, but the sentry warnings that next.js may be hammering us with warnings ironically persists and hammers us with warnings 😮‍💨. SENTRY_IGNORE_API_RESOLUTION_ERROR doesn't seem to do anything

Edit: I lie. The SENTRY_IGNORE_API_RESOLUTION_ERROR flag does work when set in your local env variables. I dug around in the source code for the withSentry method and can see it referencing it before executing the console.warning. Don't know why it didn't work when I tried added the env var before or why searching the repo for that string doesn't hit that reference, but at least i've got the log squashed 😆

kamilkisiela pushed a commit to kamilkisiela/graphql-hive that referenced this issue Sep 6, 2022
#336)

* fix: set config.api.externalResolver to true in order to avoid weird behavior (getsentry/sentry-javascript#3852 (comment))

* Update packages/web/app/src/lib/api/extract-access-token-from-request.ts
lobsterkatie added a commit that referenced this issue Sep 26, 2022
As part of #5505, this applies to API route handlers the same kind of auto-wrapping we've done with the data fetchers (`withServerSideProps` and the like). Though the general idea is the same, the one extra complicating factor here is that there's a good chance the handlers get to us already wrapped in `withSentry`, which we've up until now been telling users to use as a manual wrapper. This is handled by making `withSentry` idempotent - if it detects that it's already been run on the current request, it simply acts as a pass-through to the function it wraps.

Notes:

- A new template has been created to act as a proxy module for API routes, but the proxying work itself is done by the same `proxyLoader` as before - it just loads one template or the other depending on an individual page's path.

- Doing this auto-wrapping gives us a chance to do one thing manual `withSentry` wrapping isn't able to do, which is set the [route config](https://nextjs.org/docs/api-routes/request-helpers) to use an external resolver, which will prevent next's dev server from throwing warnings about API routes not sending responses. (In other words, it should solve #3852.)
@AbhiPrasad
Copy link
Member

Resolved by #5778, which is published with 7.14.0.

@bsplosion
Copy link

For anyone who still is getting the log messages, 7.14.0 doesn't resolve the logs on its own. You actually have to go through and strip out all the workaround code you've put in, including the withSentry calls in each API endpoint and potentially the externalResolver configs as well.

Thanks for getting a solution in place, @lobsterkatie. The logging is much better now!

@MisterJimson
Copy link

I'm still seeing the messages on 7.24.2, I have removed all of my withSentry wrappers and I never added the externalResolver change.

@bsplosion
Copy link

I can confirm this is an issue on 7.32.1 but the same pattern was working fine on 7.31.1.

I didn't see anything in the diff which would indicate the breaking change, but the constant logging has definitely returned. Downgrading to 7.31.1 resolves again.

@statico
Copy link

statico commented Mar 6, 2023

Still hoping for a solution here.

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Mar 7, 2023

Hey @statico - since we've changed a lot with the SDK the last couple of versions (we are on 7.40.0 now), and even got rid of withSentry in the latest versions (we do this automatically now), if you encounter this, please open a new issue with:

  1. your nextjs version
  2. your webpack version
  3. details about your deployment setup (aws, vercel, etc.)
  4. a copy of your next.config.js as well as your sentry configs (and if possible, a reproduction of some sort)

Let's see if we can figure out whats going on here!

@getsentry getsentry locked as resolved and limited conversation to collaborators Mar 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.