test_runner: cleanup test timeout abort listener#48915

rluvaton · 2023-07-25T09:40:23Z

nodejs-github-bot · 2023-07-25T09:40:27Z

Review requested:

@nodejs/test_runner

lib/internal/test_runner/test.js

rluvaton · 2023-07-25T12:26:33Z

lib/internal/test_runner/test.js

+deferred.promise=PromisePrototypeThen(deferred.promise,()=>{
 thrownewERR_TEST_FAILURE(
 `test timed out after ${timeout}ms`,
 kTestTimeoutFailure,
 );
 });


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

lib/internal/test_runner/test.js

rluvaton · 2023-07-25T12:28:18Z

lib/internal/test_runner/test.js

+functionthrowOnAbort(){
+deferred.reject(newAbortError(undefined,{cause: signal.reason}));
+clearTimeout(timer);
 }


Done this to match the current behavior:
node/lib/timers/promises.js
Line 48 in 78f6952
reject(newAbortError(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

lib/internal/test_runner/test.js

atlowChemi · 2023-07-25T13:51:14Z

lib/internal/test_runner/test.js

+constcleanAbort=addAbortListener(signal,throwOnAbort)[SymbolDispose];
+consttimer=setTimeout(()=>{
+deferred.resolve();
+cleanAbort();
+},timeout);
+
+timer.unref();


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:
functionstopTest(timeout,signal){consttimeoutSignal=timeout!==kDefaultTimeout&&AbortSignal.timeout(timeout);constcomposedSignal=AbortSignal.any([signal,timeoutSignal].filter(Boolean));returnPromisePrototypeThen(once(composedSignal,'abort'),(e)=>{if(e.target.reason!==timeoutSignal.reason){return;}thrownewERR_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

lib/internal/test_runner/test.js

atlowChemi · 2023-07-25T15:46:27Z

lib/internal/test_runner/test.js

+constcleanAbort=addAbortListener(signal,throwOnAbort)[SymbolDispose];
+consttimer=setTimeout(()=>{
+deferred.resolve();
+cleanAbort();
+},timeout);
+
+timer.unref();


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?

lib/internal/test_runner/test.js

MoLow · 2023-07-25T18:28:19Z

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

rluvaton · 2023-07-25T18:43:18Z

@MoLow updated with your changes and modified to use ObjectDefineProperty

lib/internal/test_runner/test.js

benjamingr

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.

rluvaton · 2023-07-26T09:59:46Z

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

benjamingr · 2023-07-26T13:10:37Z

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

rluvaton · 2023-07-26T15:37:49Z

(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');functionstopTest(timeout,signal){returnaborted(signal,{});}

The test I wrote:

const{ beforeEach, afterEach, test}=require("node:test");beforeEach(()=>{global.gc();});afterEach(()=>{global.gc();});for(leti=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:

functionstopTest(timeout,signal){returnnewPromise((resolve)=>{signal.addEventListener('abort',resolve,{[kWeakHandler]: {}});setImmediate(()=>{global.gc()}).unref()});}

Maybe I'm missing something?

atlowChemi · 2023-07-26T17:06:23Z

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...

rluvaton · 2023-07-26T17:11:01Z

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

rluvaton · 2023-07-28T15:01:20Z

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

fixnodejs#48475

no longer needed after nodejs#48915 fix PR-URL: nodejs#48989 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

RafaelGSS · 2023-08-17T19:10:45Z

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

fixnodejs#48475 PR-URL: nodejs#48915 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

no longer needed after nodejs#48915 fix PR-URL: nodejs#48989 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

rluvaton · 2023-08-18T10:07:25Z

Backport PR:

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

fixnodejs#48475 PR-URL: nodejs#48915 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

no longer needed after nodejs#48915 fix PR-URL: nodejs#48989 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

fix#48475 PR-URL: #48915 Backport-PR-URL: #49225 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

no longer needed after #48915 fix PR-URL: #48989 Backport-PR-URL: #49225 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]>

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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]>

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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]>

fix#48475 PR-URL: #48915 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

no longer needed after #48915 fix PR-URL: #48989 Backport-PR-URL: #49225 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]>

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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]>

fix #48475 PR-URL: nodejs/node#48915 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]>

fix #48475 PR-URL: nodejs/node#48915 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

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 <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]>

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

lib/internal/test_runner/test.js Outdated Show resolvedHide resolved

rluvaton commented Jul 25, 2023
View reviewed changes

MoLow reviewed Jul 25, 2023
View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolvedHide resolved

rluvaton commented Jul 25, 2023
View reviewed changes

MoLow reviewed Jul 25, 2023
View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolvedHide resolved

MoLow reviewed Jul 25, 2023
View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolvedHide resolved

atlowChemi reviewed Jul 25, 2023
View reviewed changes

ljharb reviewed Jul 25, 2023
View reviewed changes

lib/internal/test_runner/test.js Outdated Show resolvedHide resolved
lib/internal/test_runner/test.js Outdated Show resolvedHide resolved

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

lib/internal/test_runner/test.jsShow resolvedHide resolved

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
fixnodejs#48475

added test
8657bc3

fix with custom timeout as well
f19a8c2

RafaelGSS added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. 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 Indicate that the PR has an open backport and removed backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. 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 PRs backported to the v20.x-staging branch. and removed backport-open-v20.x Indicate that the PR has an open backport 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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

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 <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>

targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged

Uh oh!

test_runner: cleanup test timeout abort listener#48915

test_runner: cleanup test timeout abort listener #48915

Uh oh!

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!

rluvatonJul 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!

atlowChemiJul 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!

atlowChemiJul 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

Milestone

rluvaton commented Jul 25, 2023•
edited
Loading

rluvatonJul 25, 2023•
edited
Loading

atlowChemiJul 26, 2023•
edited
Loading

atlowChemiJul 26, 2023•
edited
Loading

rluvaton commented Jul 26, 2023•
edited
Loading

rluvaton commented Jul 28, 2023•
edited
Loading