Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

src: emit 'beforeExit' event on process object #6316

Closed
wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

Unlike the 'exit' event, this event allows the user to schedule more
work and thereby postpone the exit. That also means that the
'beforeExit' event may be emitted many times, see the attached test
case for an example.

Refs #6305.

TODO: Still needs documentation.
Suggested reviewers: @tjfontaine @sam-github

Unlike the 'exit' event, this event allows the user to schedule more
work and thereby postpone the exit.  That also means that the
'beforeExit' event may be emitted many times, see the attached test
case for an example.

Refs nodejs#6305.
@sam-github
Copy link

There's no docs, is this intended to be not-for-use-by-the-public?

There is no test that process.exit() cannot be trapped and ignored via this event, something that came up in review of #5583, see test/simple/test-beforeexit-event-exit.js

Its not clear due to lack of docs and above test whether this is functionally identical to #5583, with code restructured to your preferences, or whether it differs slightly in function.

@bnoordhuis
Copy link
Member Author

There's no docs, is this intended to be not-for-use-by-the-public?

See the TODO in the description: TODO: Still needs documentation.

There is no test that process.exit() cannot be trapped and ignored via this event

I added another test.

Its not clear due to lack of docs and above test whether this is functionally identical to #5583, with code restructured to your preferences, or whether it differs slightly in function.

It's the same thing but it a) applies to the current HEAD, and b) doesn't require changes to libuv.

@tjfontaine
Copy link

I'm still +1 on this idea, and agree with most of what I said in #5583 (comment) and @sam-github's initial documentation is quite a good start for this, I vote we just include it in your commit

I am not sure if maybe in addition to the exitCode we should also provide a state counter indicating the number of times the loop has been resurrected, since it seems like a common pattern for consumers and useful state to have when debugging.

@bnoordhuis
Copy link
Member Author

I am not sure if maybe in addition to the exitCode we should also provide a state counter indicating the number of times the loop has been resurrected

Never cement in the API what the user can (easily) keep track of herself.

Stealing^Wincluding Sam's documentation is a good idea, I'll add it.

@tjfontaine
Copy link

It's not necessarily fool proof to keep track of this by yourself, because (oh god why would I say this) someone could load another module which also watches and then there's two opinions on how many times this has happened, even if we don't expose it to the callback I think keeping the state is a good idea

@sam-github
Copy link

@bnoordhuis ok, should I close the other PR, or will you?

@bnoordhuis bnoordhuis mentioned this pull request Oct 8, 2013
@trevnorris
Copy link

I'm a little behind on the implementation, and I'm probably missing
something, but I'm assuming if there are two callbacks assigned then both
will get called. And two different counters in two different scopes
incrementing when their callbacks are called should always have the same
result.

@tjfontaine
Copy link

Having within scope the expected value is not the same thing as being
able to check the state from a global when debugging a process

@trevnorris
Copy link

If we insist on implementing this, please do so by setting a counter (e.g.
size_t) in env.h and setting a getter for JS. This is a value that won't be
accessed much and that approach will have the least performance impact.

@indutny
Copy link
Member

indutny commented Feb 28, 2014

Thank you, landed in a2eeb43

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants