Confirm processes are no longer visible before returning #37
Conversation
Autre31415
changed the title
|
Are you still interested in finishing this? |
|
I've merged in the latest and made some small tweaks. This is ready for review. |
Autre31415
changed the title
index.js
Outdated
| let i = 0; | ||
|
|
||
| return new Promise(resolve => { | ||
| const checker = setInterval(async () => { |
There was a problem hiding this comment.
It would be better to use setTimeout and call it recursively. Now, if processExists for some reason takes more than 100 ms, it will be called too often.
index.js
Outdated
| clearInterval(checker); | ||
| } | ||
|
|
||
| // Give up after 2 seconds |
There was a problem hiding this comment.
Why 2 seconds? And the 2 seconds should be a constant, so it can easily be changed if needed.
index.js
Outdated
| const killChecker = async input => { | ||
| let i = 0; | ||
|
|
||
| return new Promise(resolve => { |
There was a problem hiding this comment.
Almost anytime you want to use new Promise, there's a better way. You can just recursively await instead.
There was a problem hiding this comment.
In this case new promise is being used to convert the callback nature of setInterval (would also apply in the setTimeout format) into a promise that we can then await. Could you expand a bit on how we could go about this conversion without using new promise?
There was a problem hiding this comment.
Per https://github.com/sindresorhus/fkill/pull/37/files#r383127369, you use setTimeout instead and make it recursive by calling itself.
https://stackoverflow.com/questions/46242600/recursive-async-function-in-javascript
index.js
Outdated
| return new Promise(resolve => { | ||
| const checker = setInterval(async () => { | ||
| i++; | ||
| const exists = await processExists(input); |
There was a problem hiding this comment.
If processExists rejects, it will cause an uncaught exception as it's not caught anywhere, since you're using new Promise.
|
This behavior needs to be documented in the readme and index.d.ts. |
Autre31415
force-pushed
the
process-polling
branch
from
6273ca2
to
87231b6
Compare
Autre31415
force-pushed
the
process-polling
branch
from
bdd6a40
to
cd3c61f
Compare
| return; | ||
| } | ||
|
|
||
| const seconds = options.verifyTimeout || 2; |
There was a problem hiding this comment.
This will result in 2 if the user specifies 0 as timeout.
| const exists = await processExists(input); | ||
|
|
||
| if (exists && checks > 0) { | ||
| timeout(); |
There was a problem hiding this comment.
What's the purpose of this? Since it's not awaited.
There was a problem hiding this comment.
Can you add a test that ensures it takes the time given in verifyTimeout? You can use https://github.com/sindresorhus/delay#delayrangeminimum-maximum-options for this, to give it a slight threshold.
| Type: `boolean`\ | ||
| Default: `true` | ||
|
|
||
| Perform a rigorous check that the process is no longer visible to the system before returning. |
There was a problem hiding this comment.
| Perform a rigorous check that the process is no longer visible to the system before returning. | |
| Perform a rigorous check that the process is no longer running before returning. |
"visible" sounds weird. Makes me think it could be "hidden".
|
|
||
| const fkillProxy = proxyquire('.', {'process-exists': processExistsStub}); | ||
| await fkillProxy(pid, {verify: false, force: true}); | ||
| t.true(count === 0); |
There was a problem hiding this comment.
| t.true(count === 0); | |
| t.is(count, 0); |
| } | ||
|
|
||
| const seconds = options.verifyTimeout || 2; | ||
| let checks = seconds * 10; |
There was a problem hiding this comment.
This logic is not sound. You cannot guarantee how long things take. It's better to just continue checking until you measure using Date() that 2 seconds have passed.
|
|
||
| function timeout() { | ||
| return new Promise(resolve => setTimeout(resolve)); | ||
| } |
There was a problem hiding this comment.
This can be top-level and be an inline arrow function.
|
|
||
| return checker(); | ||
| }; | ||
|
|
||
| const fkill = async (inputs, options = {}) => { |
There was a problem hiding this comment.
Better to set defaults upfront.
| const fkill = async (inputs, options = {}) => { | |
| const fkill = async (inputs, {verify = true, veryifyTimeout = 2} = {}) => { |
|
@Autre31415 Bump |
This is a simple implementation of a confidence checker in response to my findings in #10.
Notes:
verifyoption, which allows this feature to be toggled (trueby default).verifyTimeoutoption, which allows the max time for checking to be configured in seconds (2by default).index.d.tsI noticed thesilentoption was missing so I tossed that in as well.noopProcessKilledfunction in tests along with its associateddelaydev dependency.