fix: stop command not working on Windows OS

[L2jLiga]

Fixes #312

[L2jLiga]

Draft because tests need to be fixed, also I don't really like this solution because it looks like if log level "ESLINT_D_STOP" then shutdown which is hacky 😅

[abelokon0711]

@L2jLiga Thanks so much for submitting this PR! Quick note—SIGTERM is also sent during system shutdowns or reboots. If I’m understanding correctly, with the current implementation, shutdown() wouldn’t be called in those situations, so the config file wouldn’t be properly removed. This could cause unexpected behavior when the system restarts and eslint_d is used again, as we've noticed in our observations.

[L2jLiga]

@L2jLiga Thanks so much for submitting this PR! Quick note—SIGTERM is also sent during system shutdowns or reboots. If I’m understanding correctly, with the current implementation, shutdown() wouldn’t be called in those situations, so the config file wouldn’t be properly removed. This could cause unexpected behavior when the system restarts and eslint_d is used again, as we've noticed in our observations.

That's correct, but I don't see big problem here because eslint_d checks if config is outdated before forward command to daemon:

eslint_d.js/bin/eslint_d.js

Lines 56 to 67 in 2528299

	default:
	if (config && config.hash !== hash) {
	await stopDaemon(resolver, config);
	config = null;
	}
	if (!config) {
	config = await launchDaemon(resolver, hash);
	if (!config) {
	return;
	}
	}
	await forwardToDaemon(resolver, config);

But it can break eslint_d start command because it didn't check for hash and will just output that eslintd is already running

eslint_d.js/bin/eslint_d.js

Lines 33 to 39 in 2528299

	case 'start':
	if (config) {
	console.log('eslint_d: Already running');
	} else {
	await launchDaemon(resolver, hash);
	}
	return;

@mantoni wdyt? should we adjust start command to handle such case? Or am I missing something and it's already handled?

[mantoni]

Hey, thank you for contributing!

I'm a little puzzled though since the node docs say that kill should work on child processes. Can you elaborate why that doesn't work in eslint_d?

[L2jLiga]

I'm a little puzzled though since the node docs say that kill should work on child processes. Can you elaborate why that doesn't work in eslint_d?

Sure, process.kill works fine on Windows, but as stated in docs

Sending SIGINT, SIGTERM, and SIGKILL will cause the unconditional termination of the target process

This actually means that process will be just force-killed and it will never receive "SIGTERM" event here:

eslint_d.js/lib/daemon.js

Line 46 in 2528299

process.on('SIGTERM', shutdown);

hence this code won't be called:

eslint_d.js/lib/daemon.js

Lines 74 to 93 in 2528299

	function shutdown() {
	if (idle_timeout) {
	clearTimeout(idle_timeout);
	idle_timeout = null;
	}
	if (ppid_interval) {
	clearInterval(ppid_interval);
	ppid_interval = null;
	}
	if (watcher) {
	watcher.close();
	watcher = null;
	}
	server.close(() =>
	removeConfig(resolver).then(() => {
	// eslint-disable-next-line n/no-process-exit
	process.exit(0);
	})
	);
	}

and hence this promise never resolved:

eslint_d.js/lib/launcher.js

Line 55 in 2528299

await waitForConfig(resolver.base);

that's why eslint_d stop never completes

[mantoni]

Thank you for the detailed explanation. I think your proposed solution is fine, and the "hack" of using the color parameter is fine with me.

That's correct, but I don't see big problem here because eslint_d checks if config is outdated before forward command to daemon

This is only partially true. Outdated config means that the hash changed which means package.json (or some other related file) has changes. When rebooting a machine the hash doesn't change. Currently eslint_d is relying on the config to be removed by the child process.

When eslint_d is used with a config and the daemon isn't there anymore, it runs into this error handler:

eslint_d.js/lib/forwarder.js

Lines 93 to 101 in 2528299

	.on('error', async (err) => {
	if (err['code'] === 'ECONNREFUSED') {
	console.error(`eslint_d: ${err} - removing config`);
	await removeConfig(resolver);
	} else {
	console.error(`eslint_d: ${err}`);
	}
	process.exitCode = 1;
	});

This cleans up the config and a subsequent invocation will then start the daemon again. However, the first invocation will fail. To make eslint_d more robust, the forwarder could return a status flag and we could automatically start the daemon and forward the current request to the new instance.

should we adjust start command to handle such case? Or am I missing something and it's already handled?

You're right, the start command blindly trusts that the existence of the config file means that the daemon is running. One solution could be to add another command that we can send to the daemon (e.g. ESLINT_D_PING). If the connection cannot be established, the cleanup / restart logic could be triggered.

[L2jLiga]

@mantoni I've added/updated tests.
In general it's ready for review, the only question is should I try to fix integration tests instability on Windows if we could just exclude them from CI/CD?

We can try to increase integration tests timeout to solve most of instability issues, but time-to-time they fail for weird reasons

[L2jLiga]

To make eslint_d more robust, the forwarder could return a status flag and we could automatically start the daemon and forward the current request to the new instance.

I think it should go in separate PR/commit

[L2jLiga]

I've disabled integration tests on Windows, CI is green now 😄

[L2jLiga]

Hi @mantoni , any comments on this?

[mantoni]

📦 Released in v14.0.4. Thanks a lot for this!

Sorry for the long wait. I've been under water at work. I added test coverage in #314 and noticed that we can actually unit test both code branches by stubbing os.platform().