[santigimeno]

Fixes: nodejs/node-v8#227

Suggested change

	if (isolate_->IsExecutionTerminating())
	if (is_stopping())

?

I already tried that approach but it made a number of tests not to exit because, I think, they need that some immediates that don't call into JS to run before exiting (also, apparently calling TerminateExecution() doesn't make an immediate call to IsExecutionTerminating() to return true, it may take a while before it does).

Well yeah, looking at this code, it seems like this isn’t the right place to just return; without calling the remaining callbacks. Some of them perform cleanup operations that shouldn’t be skipped, it’s rather up to the callbacks to ensure that they don’t call into JS.

Can you leave a comment here about why this is necessary? I’d definitely like to avoid a change like this, esp. considering that this method should never attempt to call into JS anyway, so if it’s about signaling something specific to callers then it’s probably worth thinking about whether there’s another way of doing this. (If it’s about the HandleScope then I’d suggest to conditionally omit that scope, but that seems like it would cause trouble for us in other places as well.)

Yes, it was because of the HandleScope and I agree this is not the place to make this change. I'm trying another approach in the next patch.

@addaleax I updated the code. Could you PTAL? Thanks!

Is this something that we can land on main after verifying that it fixes canary or do we have to wait?

CI: https://ci.nodejs.org/job/node-test-pull-request/46666/

Debug build is green 🎉

https://ci.nodejs.org/job/node-test-commit-arm-debug/4084/

Is this something that we can land on main after verifying that it fixes canary or do we have to wait?

I don't think there's a reason this can't land on main first.

Ok, then would you like to retarget this to main (or open another PR if that's easier)?

@targos done

@nodejs/cpp-reviewers

CI: https://ci.nodejs.org/job/node-test-pull-request/46680/

Landed in d2cf01d

I think this may be causing access to uninitialized memory because this code may run during construction of Environment but before the atomics have been initialized? Sending #48566.