test_runner: cleanup test timeout abort listener by rluvaton · Pull Request #48915 · nodejs/node · GitHub

Review requested:

@nodejs/test_runner

I wanted to put this in the setTimeout but for some reason, the test-runner-output.mjs test failed for the describe_it file with some missing stack trace

Done this to match the current behavior:

node/lib/timers/promises.js

Line 48 in 78f6952

reject(new AbortError(undefined, { cause: signal?.reason }));

but we should align it with just using an abort signal with no timeout - no error being thrown or throw an abort error

Could/Should this be an AbortSignal.any([signal, AbortSignal.timeout(timeout)]) etc?

I don't think it would change anything as the timer won't get cleaned...

What do you mean about the timer not getting clean?
The AbortSignal.any uses weak refs, so once this is GCed it would clear the timeout...

I think this suggestion could potentially reduce the complexity here, and also make it easier to land on v18, which doesn't have dispose & addAbortListener at the moment

WDYT?

thank you! this is a much cleaner solution! the only problem here is that we create 2 new AbortSignal (AbortSignal.any and AbortSignal.timeout) for every describe/test/hook run, which can be bad for performance...

@MoLow what do you think? should we remove the addAbortListener and dispose in order to support v18 and v16?

I don't think this actually simplifies 😬

@rluvaton I had something like this in mind:

function stopTest(timeout, signal) {
  const timeoutSignal = timeout !== kDefaultTimeout && AbortSignal.timeout(timeout);
  const composedSignal = AbortSignal.any([signal, timeoutSignal].filter(Boolean));
  return PromisePrototypeThen(once(composedSignal, 'abort'), (e) => {
    if (e.target.reason !== timeoutSignal.reason) {
      return;
    }
    throw new ERR_TEST_FAILURE(
      `test timed out after ${timeout}ms`,
      kTestTimeoutFailure,
    );
  })
};

But current solution works for me 🙂

If we accept the extra creation of an abort signal I highly prefer that

I assume we would want to profile both and decide based on that?
@MoLow @benjamingr what is your opinion here?

I've created a benchmark PR and after it is merged we can check different solutions:

benchmark: add benchmarks for the test_runner #48931

What do you mean about the timer not getting clean?
The AbortSignal.any uses weak refs, so once this is GCed it would clear the timeout...

I think this suggestion could potentially reduce the complexity here, and also make it easier to land on v18, which doesn't have dispose & addAbortListener at the moment

WDYT?

@rluvaton see this implementation, I think a little cleaner and performant - WDYT? fa48543

@MoLow updated with your changes and modified to use ObjectDefineProperty

I don't like the promise+disposer in one object code pattern here + I don't like the explicit promise construction here.

We're core, we can "cheat", we can add the listener with [kWeakHandler]: test and it will cleanup the listener when the test is done.

We're core, we can "cheat", we can add the listener with [kWeakHandler]: test and it will cleanup the listener when the test is done.

to which listener? the abort listener? because using the aborted function from internal/abort_controller and passing the test as the resource did not work to clean up the abort listener

The issue is the listener won't be removed right? kWeakListener means "remove the listener when that resource is gcd"

(I removed the timeout support so I try to make it work for the simple case)

I understand, but when I tried changing the stopTest function to using aborted (which under the hood use the kWeakHandler):

const { aborted } = require('internal/abort_controller');

function stopTest(timeout, signal) {
  return aborted(signal, {});
}

The test I wrote:

const { beforeEach, afterEach, test} = require("node:test");
beforeEach(() => {
  global.gc();
});
afterEach(() => {
  global.gc();
});

for (let i = 1; i <= 11; ++i) {
  test(`${i}`, () => {
  global.gc();
  });
}

it still log that:

(node:60047) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit
    at [kNewListener] (node:internal/event_target:542:17)
    at [kNewListener] (node:internal/abort_controller:239:24)
    at EventTarget.addEventListener (node:internal/event_target:655:23)
    at node:internal/test_runner/test:85:12
    at new Promise (<anonymous>)
    at stopTest (node:internal/test_runner/test:84:10)
    at TestHook.run (node:internal/test_runner/test:563:21)
    at TestHook.run (node:internal/test_runner/test:772:18)
    at node:internal/test_runner/test:519:20
    at async Test.runHook (node:internal/test_runner/test:517:7)

also when doing this it had the same effect:

function stopTest(timeout, signal) {
  return new Promise((resolve) => {
    signal.addEventListener('abort', resolve, { [kWeakHandler]: {} });
    setImmediate(() => {
      global.gc()
    }).unref()
  });
}

Maybe I'm missing something?

Maybe I'm missing something?

@rluvaton if we want to keep using the original signal we could keep updating kMaxEventTargetListeners, and since this AFAIK this signal is created internally we can probably safely assume we know how many listeners are being added...

The problem is that we are not cleaning up the abort listener (see that it still happening even when we GC manually)

The reason why the warning still printed was because of #48951

fix nodejs#48475

Due to fact, #48877 didn't land cleanly on v20.x-staging. This PR somehow, depends on it. So we'll need a manual backport.

Reference: https://github.com/nodejs/node/blob/main/doc/contributing/backporting-to-release-lines.md

Backport PR:

[v20.x backport] backport multiple test runner related prs #49225

fix nodejs#48475 PR-URL: nodejs#48915 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

no longer needed after nodejs#48915 fix PR-URL: nodejs#48989 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

fix #48475 PR-URL: #48915 Backport-PR-URL: #49225 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

no longer needed after #48915 fix PR-URL: #48989 Backport-PR-URL: #49225 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

There is an `if` statement that likely should have been an `else` in the original PR. Refs: nodejs#48915

There is an `if` statement that likely should have been an `else` in the original PR. Refs: #48915 PR-URL: #49943 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>

fix #48475 PR-URL: #48915 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

no longer needed after #48915 fix PR-URL: #48989 Backport-PR-URL: #49225 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

There is an `if` statement that likely should have been an `else` in the original PR. Refs: #48915 PR-URL: #49943 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>

There is an `if` statement that likely should have been an `else` in the original PR. Refs: nodejs#48915 PR-URL: nodejs#49943 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>

fix #48475 PR-URL: nodejs/node#48915 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

There is an `if` statement that likely should have been an `else` in the original PR. Refs: nodejs/node#48915 PR-URL: nodejs/node#49943 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>

fix #48475 PR-URL: nodejs/node#48915 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

There is an `if` statement that likely should have been an `else` in the original PR. Refs: nodejs/node#48915 PR-URL: nodejs/node#49943 Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: LiviaMedeiros <livia@cirno.name>

nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jul 25, 2023

rluvaton marked this pull request as ready for review July 25, 2023 09:44

rluvaton commented Jul 25, 2023

View reviewed changes

Comment thread lib/internal/test_runner/test.js Outdated

rluvaton commented Jul 25, 2023

View reviewed changes

MoLow reviewed Jul 25, 2023

View reviewed changes

Comment thread lib/internal/test_runner/test.js Outdated

rluvaton commented Jul 25, 2023

View reviewed changes

MoLow reviewed Jul 25, 2023

View reviewed changes

Comment thread lib/internal/test_runner/test.js Outdated

MoLow reviewed Jul 25, 2023

View reviewed changes

Comment thread lib/internal/test_runner/test.js Outdated

atlowChemi reviewed Jul 25, 2023

View reviewed changes

ljharb reviewed Jul 25, 2023

View reviewed changes

Comment thread lib/internal/test_runner/test.js Outdated

Comment thread lib/internal/test_runner/test.js Outdated

MoLow approved these changes Jul 25, 2023

View reviewed changes

MoLow requested a review from benjamingr July 25, 2023 18:56

atlowChemi reviewed Jul 26, 2023

View reviewed changes

Comment thread lib/internal/test_runner/test.js Outdated

benjamingr requested changes Jul 26, 2023

View reviewed changes

rluvaton mentioned this pull request Jul 26, 2023

benchmark: add benchmarks for the test_runner #48931

Merged

benjamingr approved these changes Jul 29, 2023

View reviewed changes

rluvaton added 3 commits July 29, 2023 20:31

test_runner: cleanup test timeout abort listener

272ff66

fix nodejs#48475

added test

8657bc3

fix with custom timeout as well

f19a8c2

RafaelGSS added the backport-requested-v20.x label Aug 17, 2023

rluvaton mentioned this pull request Aug 18, 2023

[v20.x backport] backport multiple test runner related prs #49225

Closed

rluvaton added backport-open-v20.x and removed backport-requested-v20.x labels Aug 18, 2023

UlisesGascon mentioned this pull request Sep 10, 2023

v20.7.0 proposal #49592

Merged

cjihrig added a commit to cjihrig/node that referenced this pull request Sep 28, 2023

test_runner: replace spurious if with else

1bb2d08

There is an `if` statement that likely should have been an `else` in the original PR. Refs: nodejs#48915

cjihrig mentioned this pull request Sep 28, 2023

test_runner: replace spurious if with else #49943

Merged

targos added backported-to-v20.x and removed backport-open-v20.x labels Nov 12, 2023

targos pushed a commit that referenced this pull request Nov 27, 2023

test_runner: cleanup test timeout abort listener

67d7fad

fix #48475 PR-URL: #48915 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

targos pushed a commit that referenced this pull request Nov 27, 2023

test: remove --no-warnings flag in test_runner fixtures

7e714e6

no longer needed after #48915 fix PR-URL: #48989 Backport-PR-URL: #49225 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

test_runner: cleanup test timeout abort listener by rluvaton · Pull Request #48915 · nodejs/node · GitHub

Uh oh!

Uh oh!

test_runner: cleanup test timeout abort listener - #48915

test_runner: cleanup test timeout abort listener#48915nodejs-github-bot merged 13 commits intonodejs:mainnodejs/node:mainfrom rluvaton:hooks-generate-MaxListenersExceededWarningrluvaton/node:hooks-generate-MaxListenersExceededWarningCopy head branch name to clipboard

Conversation

rluvaton commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 25, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rluvaton Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atlowChemi Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atlowChemi Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MoLow commented Jul 25, 2023

Uh oh!

rluvaton commented Jul 25, 2023

Uh oh!

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

rluvaton commented Jul 26, 2023

Uh oh!

benjamingr commented Jul 26, 2023

Uh oh!

rluvaton commented Jul 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atlowChemi commented Jul 26, 2023

Uh oh!

rluvaton commented Jul 26, 2023

Uh oh!

rluvaton commented Jul 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RafaelGSS commented Aug 17, 2023

Uh oh!

rluvaton commented Aug 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

test_runner: cleanup test timeout abort listener#48915
nodejs-github-bot merged 13 commits into
nodejs:mainfrom
rluvaton:hooks-generate-MaxListenersExceededWarning

rluvaton commented Jul 25, 2023 •
edited
Loading

rluvaton Jul 25, 2023 •
edited
Loading

atlowChemi Jul 26, 2023 •
edited
Loading

atlowChemi Jul 26, 2023 •
edited
Loading

rluvaton commented Jul 26, 2023 •
edited
Loading

rluvaton commented Jul 28, 2023 •
edited
Loading