GeezerVison

CI: https://ci.nodejs.org/job/node-test-pull-request/54523/

CI: https://ci.nodejs.org/job/node-test-pull-request/54528/

CI: https://ci.nodejs.org/job/node-test-pull-request/54539/

@nodejs/fs @nodejs/performance @nodejs/platform-windows can you review?

What do you mean by platform dependent timeout?

async/away has some overhead and the idea of this benchmark is to test the performance of the callback version, so there is no reason to change this behavior.

Another approach for setTimeout could be setImediate, but I didn't test if could handle the race condition.

These changes are meant to fix the error that happens in these lines:

Due to race condition, the file cannot be deleted because the readline still locking the file.

Maybe we could remove the tmpdir dependency and create and delete manually the temp folders, but I think we will have the same issue and the solution will be the same (setTimeout).

What do you mean by platform dependent timeout?

https://github.com/nodejs/node/tree/main/test/common#platformtimeoutms

async/away has some overhead and the idea of this benchmark is to test the performance of the callback version, so there is no reason to change this behavior.

I guarantee you it has less or as much overhead as the "create a promise then go through extra closure and function" version we currently use.

Another approach for setTimeout could be setImediate, but I didn't test if could handle the race condition.

We should never rely on timers for dealing with race conditions typically. The problem here is design, we should delete the file when the test starts and not when it ends..

@RafaelGSS

Cant we have a after helper, like mocha/chai/tap has, in the benchmark framework and run it after the benchmark finished?

What if the user stops the benchmark in the middle (or similarly in CI?)?

What's the issue with only doing the cleanup before the test and not after?

@benjamingr Doing it this way, it will clean up only during the start of the benchmark, and then the file will be there without being cleaned.

It may or may not be a problem, but if we went this way we would need to remove the reference to tempdir.refresh because they automatically install the process.on('exit') cleanup hook.

About stopping the CI in the middle, the file is cleaned because of process.on('exit') but this can lead to the same error, in this context, the error doesn't matter since we are canceling the execution.

But I will try to use platform-specific timeout and see how it behaves with async/await.

Doing it this way, it will clean up only during the start of the benchmark, and then the file will be there without being cleaned.

It may or may not be a problem

It's not (at least shouldn't be) it's a tmp dir, it's transient storage by design. It's "fine" to leave trash there.

But I will try to use platform-specific timeout and see how it behaves with async/await.

Sure but I would strongly prefer it that we don't use a timers based solution since that inherently depends on how slow the machine running the benchmark/test is which means harder-to-track-and-fix flakes.

@benjamingr Just to summarize the changes, now I wait for all the concurrent timers to finish until really stop the benchmark, in this approach I don't relly on platform dependant timeouts.

CI: https://ci.nodejs.org/job/node-test-pull-request/54659/

CI: https://ci.nodejs.org/job/node-test-pull-request/54685/

CI: https://ci.nodejs.org/job/node-test-pull-request/54691/

@benjamingr I see you asked for some changes here, are you still blocking this to land?

CI: https://ci.nodejs.org/job/node-test-pull-request/54730/

CI: https://ci.nodejs.org/job/node-test-pull-request/54785/

CI: https://ci.nodejs.org/job/node-test-pull-request/54789/

CI: https://ci.nodejs.org/job/node-test-pull-request/54792/

Landed in 33c87ec