WebReflection / dblite

sqlite for node.js without gyp problems
MIT License
209 stars 34 forks source link

Do not detach child process #56

Closed sam-github closed 4 years ago

sam-github commented 4 years ago

The comments said "asynchronous", but that's not what detached means, it allows the child to live beyond the parent under some conditions (exact conditions vary between Windows and non-Windows). sqlite3 living longer than the parent process is probably not a good thing.

WebReflection commented 4 years ago

CI not happy, and this module has been like this for a very long time ... do you have an actual concern or this PR comes out of the blue?

sam-github commented 4 years ago

CI is red because dblite uses es6 syntax that isn't supported on the node versions under test, check the travis logs, or repro locally:

s/dblite (master u=) % nvm i 4                  
v4.9.1 is already installed.
Now using node v4.9.1 (npm v2.15.11)
s/dblite (master u=) % node build/dblite.node.js
/home/sam/s/dblite/build/dblite.node.js:929
    const {strings, values} = statement;
          ^

SyntaxError: Unexpected token {

package.json's claim of supporting 0.6 onwards isn't quite right anymore.

I was looking at dblite for other reasons, but don't have a specific issue, so feel free to close this if its just noise.

Affect is that it is possible that users can get zombie processes if the parent dies with detached children, but I can't prove that easily, and its possible that sqlite3 will always exit anyhow when its parent exits and stdin closes.

WebReflection commented 4 years ago

I was looking at dblite for other reasons, but don't have a specific issue, so feel free to close this if its just noise.

don't get me wrong, if there is a real issue I'm happy to accept MRs, but this project is in maintenance mode, as it's been working for years as is, so I don't want to change stuff just because of theoretical issue that has never manifested in all these days, months, years.

I hope this makes sense, but I'm also going on vacation, so that if I'll find any free time to better look at this, fix CI, update what's needed to updated, and change the detached part, and verify it doesn't break anything, I will.

sam-github commented 4 years ago

No sweat, I know what a pain maintaining stuff is. You might want to cleanup your CI and package.json specs, but if no one is complaining, then leaving it alone is a good option.

WebReflection commented 4 years ago

So, all good now, process is not detached, dependencies updated, package cleaned without useless template files.