open handle in @sentry/node v6.9.0

[dnalborczyk]

• Review the documentation: https://docs.sentry.io/
• Search for existing issues: https://github.com/getsentry/sentry-javascript/issues
• Use the latest release: https://github.com/getsentry/sentry-javascript/releases
• Provide a link to the affected event from your Sentry account

Package + Version

• @sentry/browser
• @sentry/node
• raven-js
• raven-node (raven for node)
• other:

Version:

6.9.0

node.js version v16.5.0

Description

v6.9.0 introduced an open handle when running with jest tests.

I was able to pin down the commit causing the issue: #3758

also tried the latest next version (6.10.0-beta.3). issue still persists.

[AbhiPrasad]

Could you provide the logs from jest?

Just to get some context, why are you initing Sentry in your tests? Does the issue still persist if you set autoSessionTracking option to be false in Sentry.init()?

[Mr0grog]

I encountered the same issue, and the only two solution I found that solved the problem were:

• Set autoSessionTracking: false in Sentry.init()

• Add a setup module to my Jest tests (using the setupFilesAfterEnv config option) that calls Sentry.close() after the tests. i.e. the module looks like:

  import * as Sentry from "@sentry/node";
  
  afterAll(async () => {
    await Sentry.close(2000);
  });

When using Jest’s --detectOpenHandles option, the problem appear to the the interval started in the SessionFlusher constructor:

  ●  Timeout

      39 | app.set("port", process.env.PORT || 3000);
      40 | app.enable("trust proxy");
    > 41 | app.use(Sentry.Handlers.requestHandler());
         |                         ^
      42 | app.use(logRequest);
      43 | app.use(compression());
      44 | app.use(bodyParser.json({ limit: "500kb" }));

      at new SessionFlusher (node_modules/@sentry/hub/src/sessionflusher.ts:31:24)
      at NodeClient.Object.<anonymous>.NodeClient.initSessionFlusher (node_modules/@sentry/node/src/client.ts:101:30)
      at Object.requestHandler (node_modules/@sentry/node/src/handlers.ts:386:12)
      at Object.<anonymous> (src/app.ts:41:25)
      at Object.<anonymous> (test/api.edge.test.ts:9:1)

Constructor code referenced by the stack trace:

sentry-javascript/packages/hub/src/sessionflusher.ts

Lines 28 to 33 in ba7ca16

	public constructor(transport: Transport, attrs: ReleaseHealthAttributes) {
	this._transport = transport;
	// Call to setInterval, so that flush is called every 60 seconds
	this._intervalId = setInterval(() => this.flush(), this.flushTimeout * 1000);
	this._sessionAttrs = attrs;
	}

…so that makes the above solutions make sense and the issue relatively clear. Nothing ever stops the SessionFlusher until the Sentry client is closed.

While the solutions I came up with work, it doesn’t seem great that simply including Sentry in a test (especially when SENTRY_DSN is not configured so you expect it to essentially be a no-op) will cause your test process to hang unless you configure things specially. Given that flipping the default for auto session tracking was probably intentional and well-discussed, I’m not sure what the obvious solution should be.

• Perhaps session tracking or the SessionFlusher should behave slightly differently if process.env.NODE_ENV === "test"?
• Calling .unref() on the interval might also solve the issue.

[Mr0grog]

(If you’re following along in e-mail, I updated the above with the actual stack trace from Jest, which I forgot to paste originally.)

[Mr0grog]

Also: I could not reproduce this locally on my Mac (MacOS 10.15.7, Node.js 16.6.1). It only hung in CI (GitHub actions), and I’m not sure what’s different.

Edit: I figured this out — the SDK detects the value for release in CI (from the GITHUB_SHA environment variable), but not in my local environment. And if it can’t detect a release, it turns autoSessionTracking off.

sentry-javascript/packages/node/src/sdk.ts

Lines 103 to 111 in ba7ca16

	if (options.release === undefined) {
	const detectedRelease = getSentryRelease();
	if (detectedRelease !== undefined) {
	options.release = detectedRelease;
	} else {
	// If release is not provided, then we should disable autoSessionTracking
	options.autoSessionTracking = false;
	}
	}

[AbhiPrasad]

Appreciate the detailed repro + stack trace, thank you!

autoSessionTracking: false should lead to the session flusher not being created and enabled, if that still happens that is a bug on our side. Will take a look.

sentry-javascript/packages/node/src/handlers.ts

Lines 385 to 386 in ba7ca16

	if (client && isAutoSessionTrackingEnabled(client)) {
	client.initSessionFlusher();

Also, @Mr0grog would love to get more insight into why you are running Sentry in tests vs. just mocking it out à la:

jest.mock('@sentry/node', () => {
  const SentryNode = jest.requireActual('@sentry/node');
  return {
    init: jest.fn(),
    configureScope: jest.fn(),
    setTag: jest.fn(),
    setExtra: jest.fn(),
    // ...
  };
});

[Mr0grog]

autoSessionTracking: false should lead to the session flusher not being created and enabled, if that still happens that is a bug on our side.

Ah, to be clear, that works! (As in: it does successfully prevent creation of the session flusher and resolves the issue with tests hanging.) The issue for me was that the change of the default in 6.9.0 was a surprising breaking change, and that, in the past, the defaults have not created a situation where Sentry needs special treatment when shutting my program down. Now they do.

Also, @Mr0grog would love to get more insight into why you are running Sentry in tests vs. just mocking it out

To be honest, I just leave it as-is in tests for most projects, since in the vast majority of cases, it runs in the test environment with no SENTRY_DSN set, and operates mostly as a no-op. And if it’s not mocked, I stand a better (obviously not perfect) chance of catching an issue with any calls to set up Sentry scopes or capture events in my code.

Any mock I set up also has to be constantly updated to keep in sync with Sentry’s API, and while Jest offers some useful auto-mocking tools there, it’s never perfect. Checking the mocks on every update is a lot of overhead when the release cadence of Sentry is every couple weeks. So in most cases, it’s preferable not to mock it.

[Mr0grog]

(Also it was surprising that the default for autoSessionTracking behaved differently in CI that my local environment, when neither had any SENTRY_* environment variables set. After digging into Sentry’s source to understand what’s happening, I get why it works that way, but it is still a somewhat surprising, undocumented difference in the overall behavior of the library and how it impacts the rest of my program.)

[AbhiPrasad]

This has been addressed with #3954 and should be available with the next release. If you have any other feedback, please let us know - it is highly appreciated.