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

Node.js 12 "Cannot read property 'enter' of undefined" #2089

Closed
5 of 6 tasks
vvo opened this issue May 28, 2019 · 43 comments
Closed
5 of 6 tasks

Node.js 12 "Cannot read property 'enter' of undefined" #2089

vvo opened this issue May 28, 2019 · 43 comments

Comments

@vvo
Copy link

vvo commented May 28, 2019

Package + Version

  • @sentry/node
  • @sentry/integrations

Version:

@sentry/node 5.3.0
@sentry/integrations 5.3.1
Node.js 12.3.1

Description

After switching to Node 12.3.1 I started to have hard crashes of my application this way:

TypeError: Cannot read property 'enter' of undefined
    at AsyncHook.before (domain.js:76:20)
    at emitHook (internal/async_hooks.js:164:38)

Here's how we use Sentry (relevant lines only):

const express = require('express');
const app = express();
const Sentry = require('@sentry/node');
const SentryIntegrations = require('@sentry/integrations');
Sentry.init({
  dsn: process.env.SENTRY_DSN,
  environment: process.env.APP_ENV,
  release: `support@${process.env.HEROKU_RELEASE_VERSION}`,
  integrations: [
    new SentryIntegrations.ExtraErrorData(),
    new SentryIntegrations.Transaction(),
  ],
});
app.use(Sentry.Handlers.requestHandler());
app.use(Sentry.Handlers.errorHandler());
module.exports = app;

After checking our code I believe the only places where Node.js domains are used are Sentry code. This is why I am opening this issue.

Recent changes to async_handlers linked to domains: nodejs/node@04355ef#commitcomment-33702918

Let me know if you need anything else.

@vvo vvo changed the title Node 12.3.1 Node 12.3.1 "Cannot read property 'enter' of undefined" May 28, 2019
@vvo
Copy link
Author

vvo commented Jun 17, 2019

Tested with Node.js 12.4.0 too, same result, as soon as I re-add sentry initialization as shown in the previous comment, it will crash with Cannot read property 'enter' of undefined, any insights @BYK?

@vvo vvo changed the title Node 12.3.1 "Cannot read property 'enter' of undefined" Node.js 12 "Cannot read property 'enter' of undefined" Jun 17, 2019
@BYK
Copy link
Member

BYK commented Jun 17, 2019

Sorry @vvo, no insights at this point. @HazAT, @kamilogorek any ideas?

Looks like we still have a few months before Node 12 becomes the new LTS tho.

@kamilogorek
Copy link
Contributor

Node v12.4.0, wasn't able to reproduce with your code ¯_(ツ)_/¯

image

@kunal15595
Copy link

kunal15595 commented Jul 16, 2019

Getting this on node v12.6.0, sentry v5.5.0
fixed by reverting to v12.2.0

@emhagman
Copy link

I've also encountered this issue upon upgrading to NodeJS 12.6.0

@vvo
Copy link
Author

vvo commented Jul 22, 2019

@emhagman @kunal15595 are you also using express? Trying to see anything we have in common here

@kunal15595
Copy link

@emhagman
Copy link

@spanditcaa
Copy link

fyi @vvo @BYK @kamilogorek @emhagman @kunal15595 - I came across your issue here as I had the exact same error show up in my hapi servers (no sentry) with node > 12.2.

I believe it was caused by nodejs/node#28275. The fix for that issue was merged 5 days ago with nodejs/node@43e5478 and hopefully will be released soon and resolve our issue.

When an uncaught exception is thrown inside a domain, the domain is
removed from the stack as of 43a5170.
This means that it might not be kept alive as an object anymore,
and may be garbage collected before the after() hook can run,
which tries to exit it as well.

Resolve that by making references to the domain strong while it is
active.

@kamilogorek
Copy link
Contributor

Thanks @spanditcaa!

@emhagman
Copy link

emhagman commented Aug 1, 2019

Thanks @spanditcaa !

@emhagman
Copy link

emhagman commented Aug 17, 2019

For anyone curious, this landed in 12.8.0:
https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#2019-08-06-version-1280-current-bridgear

nodejs/node@727ffe4720

@emhagman
Copy link

emhagman commented Sep 9, 2019

Just experienced this same issue in 12.10 which came out on September 3rd. Not sure this is fixed.

@vvo
Copy link
Author

vvo commented Sep 10, 2019

@emhagman Yep same for me, 12.10 is triggering it too.

@elvisplaza
Copy link

triggered for me as well, 12.10 version

@emhagman
Copy link

emhagman commented Oct 7, 2019

I am going to try and take a stab at this and submit a PR since the Sentry team has decided it's not worthy of looking at yet. I'll report back!

@cearny
Copy link

cearny commented Oct 8, 2019

Is it possible that some changes have been made to the domain module in NodeJS > 12.2.0 are causing this? (which would be in my opinion a strange decision since it's deprecated so it should be "do not touch", more or less?)

I'm asking because we have almost identical crashes in an unrelated app that works swimmingly under Node < 12 and Node 12 < 12.3.0. We're using "domain" there as a sort of ambient context.

@simllll
Copy link

simllll commented Oct 8, 2019

TypeError: Cannot read property 'enter' of undefined
at AsyncHook.before (domain.js:79:20)
at emitHook (internal/async_hooks.js:164:38)
npm ERR! code ELIFECYCLE

also affected by this... any working workarround?

@kamilogorek
Copy link
Contributor

since the Sentry team has decided it's not worthy of looking at yet

The error is thrown internally within the domain implementation. I don't have any superpowers to change it. requestHandler cannot work without domain without a complete rework to async hooks, which are still in experimental phase, which makes it even worse.

@emhagman
Copy link

emhagman commented Oct 9, 2019

@kamilogorek Sorry, I was not trying to attack the team. I was more referencing that 12 is not in LTS yet so it didn't seem a priority. Other people are claiming they don't have this issue with the same versions of NodeJS that we are having issues with, so it would seem there is something specific to how some people are using it, implying there is a fix

@kamilogorek
Copy link
Contributor

@emhagman No worries. If there's a way you can reproduce it and provide the codebase, I'm more than happy to help to debug this. We need a solid repro first, as I wasn't able to break it in any way myself.

@spanditcaa
Copy link

fyi @kamilogorek the issue resurfaced for us as well. We're leaning towards moving our codebase away from domains (which is deprecated anyway) vs. expending any effort troubleshooting further.

Good luck.

@emhagman
Copy link

emhagman commented Oct 9, 2019

@kamilogorek @spanditcaa Correct if I'm wrong, but the issue is not whether we use domain in our codebase or not, but that Sentry uses it in the handlers.js file to add the req and res objects as context all the way down the request so that if an error occurs, it can send context to the Sentry service for our errors. The only modules we use that use domain are pg-native, libpq, and @sentry/node

If that's the case, removing domains from your codebase still won't fix this issue in NodeJS >= 12.4 if you're using @sentry/node

EDIT: Re-reading your comments you definitely know that's the case. Sorry!

@simllll
Copy link

simllll commented Oct 15, 2019

Is there any official statement for how we should go on with this issue? node 12 will be LTS in a few days...

@emhagman
Copy link

emhagman commented Oct 22, 2019

NodeJS 12.13.0 is now LTS. Has anyone tried that or the new 13.0.0 to see if this has been fixed in the core?

@simllll
Copy link

simllll commented Oct 22, 2019

Unfortunately it's still happening with the lts version, I have to remove the sentry express error handle for now...

@emhagman
Copy link

emhagman commented Oct 22, 2019

I guess a temporary solution is to use cls-hooked and write your own middleware to catch the errors and forward them to Sentry. We already use it for context for our logs and I'm sure the Sentry middleware isn't doing anything too crazy to enrich the data.

Normally I wouldn't care but there are supposed to be huge startup speed improvements and default memory setting for V8 based on environment in the newer 12.x versions which we'd really like to take advantage of.

Can you confirm it doesn't happen when disabling just the express error middleware?

@simllll
Copy link

simllll commented Oct 23, 2019

@emhagman yes, I can confirm it's only happening with the express middleware, we still use Sentry.captureException and captureMessage with scopes.. no issues. We have our own error handler now in place, but be awaer that breadcrumbs and co don't work as they should with this "workaround".

@emhagman
Copy link

@simllll Thanks for the info. Breadcrumbs don't work for us currently because we use pino as our logger so that's not a big deal. Sounds like this might be the solution for now

@emhagman
Copy link

I've posted a bug report on the NodeJS repo. Hopefully, someone has some insight as to why it might be happening nodejs/node#30122

@saniagh
Copy link

saniagh commented Oct 29, 2019

We're not using Sentry explicitly in our project and we've encountered this issue on our production server running in a Docker container:

  1. node:alpine - v13.0.1
  2. npm - v6.12.0

System info:
Linux <gke_pod_name> 4.14.137+ SMP Thu Aug 8 02:47:02 PDT 2019 x86_64 Linux

Unfortunately I could not get the debug log since the container was deleted when it crashed.

@emhagman
Copy link

emhagman commented Oct 29, 2019

@saniagh Check out the thread on the node side. It has to do with using https.Agent when reusing the same sockets. nodejs/node#30122

Check to see if you are calling https.Agent anywhere or any of your libraries are.

@emhagman
Copy link

emhagman commented Oct 31, 2019

@simllll @kamilogorek The issue has been found and there is a PR open! Thanks to @addaleax for taking the time to reproduce and fix it.

@saniagh This bug can happen with http or https agents so I believe this still applies to you.

nodejs/node#30122 (comment)

@emhagman
Copy link

emhagman commented Nov 6, 2019

For everyone here, the fix has landed in here nodejs/node@d26a74d

It should be released into 13.x soon and then two weeks later into 12.x so keep any eye out

@kamilogorek
Copy link
Contributor

kamilogorek commented Nov 7, 2019

@emhagman @addaleax thank you! :)

@scragg0x
Copy link

Does anyone have a workaround until the fix is in?

@emhagman
Copy link

@scragg0x Unfortunately, the best thing you can do is stay on 12.2.x until the next version is released in a week or two.

@emhagman
Copy link

For those of you using 13.x, the fix has made it into 13.2.0

https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V13.md#2019-11-21-version-1320-current-mylesborins
nodejs/node@e59cc8aad8

For those of you using 12.x, we have to wait roughly 2 more weeks for it to make it into the LTS

@simllll
Copy link

simllll commented Jan 7, 2020

anyone knows if it's landed in 12.14?

@SimenB
Copy link

SimenB commented Jan 7, 2020

Scheduled for today: nodejs/node#31069

@emhagman
Copy link

emhagman commented Jan 7, 2020

It's out! https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V12.md#12.14.1
Specifically --

[652514233f] - http,async_hooks: keep resource object alive from socket (Anna Henningsen) #30196

@kamilogorek
Copy link
Contributor

Aye! Closing it then.

@antkougblenou
Copy link

I am posting this here though it is close because I am having a similar issue.
Working on a Nextjs project with typescript, I am getting the following error:

next-dev.js?53bc:89 Error was not caught TypeError: Cannot read property 'cwd' of undefined
    at Module.eval (VM11333 parsers.js:40)
    at eval (VM11333 parsers.js:252)
    at Module../node_modules/@sentry/node/esm/parsers.js (_app.js?ts=1619773353632:3455)
    at __webpack_require__ (webpack.js?ts=1619773353632:873)
    at fn (webpack.js?ts=1619773353632:151)
    at eval (VM11332 backend.js:7)
    at Module../node_modules/@sentry/node/esm/backend.js (_app.js?ts=1619773353632:3299)
    at __webpack_require__ (webpack.js?ts=1619773353632:873)
    at fn (webpack.js?ts=1619773353632:151)
    at eval (VM11287 index.js:50)

This is a non blocking issue when forcing node in production but it is when simply running yarn dev.
The element that seems to trigger it is a Sentry.init() call in my sentry utils (following the example of sentry integration from nextjs).
Using [email protected], [email protected] and @sentry/[email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests