feat: check if daemon actually running #315
Conversation
Implemented new command - ESLINT_D_PING to check whether daemon is actually running or not. This gives us ability to check daemon and start it if required before sending user command to it.
bin/eslint_d.js
Outdated
| @@ -54,7 +60,7 @@ const command = process.argv[2]; | |||
| (await import('../lib/status.js')).status(resolver, config); | |||
| return; | |||
| default: | |||
| if (config && config.hash !== hash) { | |||
| if (config && (config.hash !== hash || !(await isAlive(config)))) { | |||
There was a problem hiding this comment.
Using isAlive in the start and status commands is a very good improvement. But calling it here adds an extra roundtrip to the server on every lint call. I ran some quick tests and it slows it down quite a bit. Given that this is the hot code execution path, I'd like to not do that here.
There is a good alternative though: The forwardToDaemon function already handles connection errors and removes the config. Instead of doing that in the function itself, forwardToDaemon could return a flag or throw an exception, so that this file here could handle connection issues, restart the service and try sending the request again. This would eliminate the need to call isAlive with the same behaviour.
Am I missing something? What do you think?
There was a problem hiding this comment.
Agree, I think using optimistic approach is good idea at least because this check will return "true" most of the time
removed JSON payload from PING command
|
Convert to draft because have to investigate why there's abandoned connections |
lib/forwarder.js
Outdated
| const socket = await connectToDaemon(config); | ||
| socket.write(JSON.stringify([config.token, command])); | ||
| if (command === SHUTDOWN_COMMAND) { | ||
| socket.destroySoon(); |
There was a problem hiding this comment.
I have no idea why destroySoon is needed, but without this method call daemon will never stop
There was a problem hiding this comment.
Hm. Maybe because we're calling shutdown() immediately which closes the server, and then we're calling .end() on the socket? Just a wild guess ...
There was a problem hiding this comment.
Yeah, as far as I understand this is related to server.close and half-open connections, for some reasons this connections could not be closed if server is closing, so I've implemented logic to await for connections completion, seems to work fine
There was a problem hiding this comment.
Also I've fixed all tests that was trivial to fix, 3 tests still failing, if you're ok with solution I'll continue fixing this tests
There was a problem hiding this comment.
After all tests are fixed I will continue with restart daemon logic in forwardToDaemon function
| try { | ||
| socket = await connectToDaemon(config); | ||
| } catch (/** @type {any} */ err) { | ||
| if (err['code'] === 'ECONNREFUSED') { |
There was a problem hiding this comment.
in 99% cases this is the only reason promise rejects, also here we didn't sent any data to socket yet, already read stdin and didn't write anything to connection, I think it's good place to integrate restart logic
| }) | ||
| ); | ||
|
|
||
| waitForConnections() |
There was a problem hiding this comment.
I'm pretty sure you're trying to work around some other issue here. server.close() actually waits for any pending connections and doesn't invoke the callback until all connections are gone.
There was a problem hiding this comment.
Yes, you're right, finally I've found root cause:
the issue was with waitForConfig, for the moment when it was called in stopDaemon - it was already removed, so watcher never get notification about
There was a problem hiding this comment.
Here's correct fix:
#316
There was a problem hiding this comment.
After #316 merged gonna rebase and cleanup things if you don't mind, otherwise can start new PR
Implemented new command - ESLINT_D_PING to check whether daemon is actually running or not. This gives us ability to check daemon and start it if required before sending user command to it.