Skip to content

fix for npm not cleaning up lockfiles on ctrl+c #4631

Closed
wants to merge 2 commits into from

5 participants

@rlidwka
rlidwka commented Feb 9, 2014

lockfile is capable of cleaning up after itself

However, npm error handler is calling process.exit() inside process.on('exit') which screws things up.

node < 0.10 doesn't have that feature, so npm on it will still be buggy... well it's a good reason for update, is it?

@rlidwka rlidwka referenced this pull request in rlidwka/yapm Feb 9, 2014
Open

need to clean up logs on exit #10

@rlidwka
rlidwka commented Feb 9, 2014

ref #4198, #4629, maybe others

@domenic
npm member
domenic commented Feb 9, 2014

Is there any way to fix this that works on current stable node versions? :-/

@rlidwka
rlidwka commented Feb 10, 2014

Is there any way to fix this that works on current stable node versions? :-/

The issue is that npm doesn't call process.on("exit") handlers that are supposed to be called after a handler in error_handler.js.

So there are two ways to solve it without depending on node.js version:

  1. move require('lockfile') at the top, so it is called before require('./utils/error_handler'). It's hacky solution that prone to errors (what about other handlers?).
  2. call all other process.on("exit") handlers manually before calling process.exit(). I expect this solution to be huge, because it'd be mocking EventEmitter internals.

Considering the fact that node.js 0.12 is expected soon enough, I assumed that it's not worth it.

It's also possible to remove that process.exit() entirely at the cost of a regression, so npm on node 0.10 would return wrong exit code. But it can break existing scripts depending on this.

@domenic
npm member
domenic commented Feb 10, 2014

call all other process.on("exit") handlers manually before calling process.exit(). I expect this solution to be huge, because it'd be mocking EventEmitter internals.

What about just process.emit("exit")?

@rlidwka
rlidwka commented Feb 10, 2014

Okay, here is the very simplified code:

process.on('exit', function(code) {
   console.log('handler1', code)
})

process.on('exit', function(code) {
   // how to change this to call both handler1 and handler2,
   // and set error code?
   process.exit(1)
})

process.on('exit', function(code) {
   console.log('handler2', code)
})

If you can painlessly solve this, it means that the same solution would work in npm.

@rlidwka rlidwka added a commit to rlidwka/yapm that referenced this pull request Mar 22, 2014
@rlidwka rlidwka Merge branch 'exitCode' of https://github.com/rlidwka/npm 5d6bffc
@isaacs
npm member
isaacs commented May 1, 2014

I'm ok with this, but it causes test/tap/ls-no-results.js to fail. It looks like it's making npm exit with a 0 status code when it should be non-zero.

@noahbuscher

I'm still having this issue. Is there a quick fix or can this please be merged? 😄

@rlidwka
rlidwka commented Jun 18, 2014

It should work now.

@othiym23

We made a bunch of changes to the error-handler as part of dealing with race conditions, and we pretty comprehensively overhauled how npm uses locks as part of npm@2. This patch no longer really makes sense in the context of the current code base, so I'm going to close it. Sorry it took so long for us to get back to you on this one, and thanks for taking the time to put it together.

@othiym23 othiym23 closed this Feb 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.