src: make Environment optional for RunAtExit

[danbev]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

src

Description of change

Currently, the RunAtExit function definition does not use the
Environment parameter. This commit makes the parameter optional
and removes it from the call site.

I noticed this when trying to figure out how to tackle the TODO for
RunAtExit which part of #4641.

[addaleax]

Hmmm – if you introduce a default parameter, shouldn’t that happen in the header where it’s declared (node.cc:212)? I would kind of expect not having a default parameter in the declaration but in the definition to throw the compiler off a bit.

CI: https://ci.nodejs.org/job/node-test-commit/5624/

[danbev]

Hmmm – if you introduce a default parameter, shouldn’t that happen in the header where it’s declared (node.cc:212)?

Sorry I got that backward when reading about default parameters and it's exactly like you stated. I'll add this to the declaration in the header and not in the definition. Thanks!

[danbev]

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

[bnoordhuis]

I'm -1 on this change, it leaks an implementation detail into a public header and those tend to be excruciatingly painful to change later on. Which TODO are you trying to address?

[danbev]

Which TODO are you trying to address?

I was looking at this TODO and noticed that the Environment parameter was not being used and though this might be an option. I'm happy to close this and continue/have a new discussion on the other issue.

[bnoordhuis]

Ah, so the idea is that at_exit_functions_ should be a per-environment property rather than a global but that is made difficult by the fact that AtExit() doesn't take an Environment pointer.

There are several ways to skin that cat though. API break, function overload, storing the environment in a thread-local, etc. Writing the code probably won't be that complicated, it's more a matter of figuring out what the best solution long-term is.

[danbev]

@bnoordhuis Thanks, I'll take a closer look and create a new PR for discussion.

[danbev]

@bnoordhuis I've taken a stab at this in #9163