benchmark: fix race condition on fs benchs

[H4ad]

Some of these benchmarks fails even on main versions of Node.js.

This issue is caused because afterRead is being called and is locking the temp file which makes the .on('exit') fails to clean the temp file.

If we wait all the concurrent timers to finish, the issue is solved.

First found this issue on: #49962

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[anonrig]

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

[benjamingr]

I'm -1 on an approach that uses setTimeout here, if we end up using setTimeout we should use platform dependent timeout probably.

In general code that does cleanup and may crash shouldn't clean up after itself but when starting which the code here already does - so I wonder if it isn't better to just remove the cleanup and rely on the test.

The benchmarks should likely use async/await instead of the callbackify-liike weirdness they do at the moment.

[H4ad]

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:

node/test/common/tmpdir.js

Lines 48 to 71 in 19b470f

	function onexit(useSpawn) {
	// Change directory to avoid possible EBUSY
	if (isMainThread)
	process.chdir(testRoot);
	try {
	rmSync(tmpPath, useSpawn);
	} catch (e) {
	console.error('Can\'t clean tmpdir:', tmpPath);
	const files = fs.readdirSync(tmpPath);
	console.error('Files blocking:', files);
	if (files.some((f) => f.startsWith('.nfs'))) {
	// Warn about NFS "silly rename"
	console.error('Note: ".nfs*" might be files that were open and ' +
	'unlinked but not closed.');
	console.error('See http://nfs.sourceforge.net/#faq_d2 for details.');
	}
	console.error();
	throw e;
	}
	}

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

[benjamingr]

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

[Uzlopak]

@RafaelGSS

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

[benjamingr]

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?

[H4ad]

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

[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

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.

[H4ad]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[mcollina]

lgtm

[mcollina]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 33c87ec