Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Should finalizers run on exit in Node.js? #125

Closed
benjamingr opened this issue May 31, 2019 · 11 comments
Closed

Should finalizers run on exit in Node.js? #125

benjamingr opened this issue May 31, 2019 · 11 comments

Comments

@benjamingr
Copy link

@mhdawson @littledan

Discussion was raised where in Java finalizers run on process exit.

In discussion people said they don't want people to put complicated logic in finalizers - so that might be a consideration.

@benjamingr
Copy link
Author

Also really easy to do from the node side (with a process exit handler)

@littledan
Copy link
Member

I'd argue, we should not run finalizers on exit in general. On the web, they will most likely not be run on page exit. Finalizers are unreliable, and trying to give them guarantees about being called on process exit might lead people to depend on them more than they should. Finalizers shouldn't be used to release external resources. If we leave them uncalled in this case, it's more likely that small tests will run into the issue, leading the bugs to get fixed.

@erights
Copy link
Contributor

erights commented May 31, 2019

I agree. Do not run them on exit.

@mhdawson
Copy link

mhdawson commented Jun 2, 2019

I don't have a strong opinion one way or the other, just asked if it would be possible since I know there is the option in Java. The thought I'd add is that it would be much easier to add it later if there ends up being a compelling case versus trying to remove it later on. For that reason, I think leaving it out to start is a reasonable way to go.

@tschneidereit
Copy link
Member

Note that doing this would almost certainly make shutdown significantly slower. I don't know much about v8's GC architecture, but would be surprised if it had to always do a full GC if an isolate is discarded. Doing that would be the only way to reliably run finalizers on exit.

What's more, you'd have to run multiple GCs until no more finalizers are run, because those might make additional objects unreachable.

@gabrielschulhof
Copy link

I would like to bring Agents into this discussion. An Agent is defined as having its own thread. With these Agents, the exit of an Agent does not coincide with the exit of the process. So, at least in the case of native data being attached to JavaScript objects, if finalizers are not run on Agent exit, there will potentially be leaks as Agents are fired up and discarded.

Now, in the browser, attaching native data to JS objects is always under the control of the browser itself so the maintainers of the browser can decide amongst themselves what mechanism to use for cleaning up the native data associated with an Agent when an Agent exits.

In contrast, in Node.js we have an open architecture where native addons can associate native data with JavaScript objects where we have no control over the nature of that data. Thus, we have to provide a standard way of informing native addons that this data needs to be freed. Until the advent of worker threads, this was pretty much beyond the scope of the project, because the native data was always freed along with the whole segment at process exit.

I guess the question is this: shall we go ahead and work on our own, Node.js standard way of informing native addons that the Agent (in our case the worker thread's node::Environment) is about to exit, or can this mechanism fit within the scope of the specification of the life cycle of an Agent? If the latter, then a second question arises: can existing finalization mechanisms, such as V8's weak callbacks, be (re-)?used in this case?

@addaleax, given your comment, please help me out if I've missed anything 🙂

@erights
Copy link
Contributor

erights commented Jul 8, 2019

I doubt that any such agent-based cleanup should be tied to weakrefs. GC is inherently unreliable/ambiguous. Agent death is clear. Any resources held by an agent should not be handled by code within the agent, because that would imply that the agent isn't dead yet.

We should also think about what the failure unit for preemptive termination, and for enabling others to cleanup after such a unit terminates.

Erlang has a good model for cleanup following process death. Their process corresponds well to our agent. However, our shared array buffers (SABs) means that our agents are not shared-nothing units, and therefore are not separable re preemptive failure. If we only treat the agent cluster as the unit of preemptive failure, then we should think about agent-cluster death, and should seek to put cleanup logic in other agent clusters.

In any case, I think non of this is relevant to weakrefs.

@gabrielschulhof
Copy link

GC is inherently unreliable/ambiguous.

Couldn't one argue that GC is fairly unambiguous when every value is about to go out of scope?

Any resources held by an agent should not be handled by code within the agent, because that would imply that the agent isn't dead yet.

So, it sounds like the entity that fired up the Agent could be made to notify of the Agent's impending doom before actually initiating such doom, but that's outside the scope of the Agent.

@erights
Copy link
Contributor

erights commented Jul 8, 2019

Couldn't one argue that GC is fairly unambiguous when every value is about to go out of scope?

Except for resurrection bugs, which the weakref design took pains to avoid.

const [z,wr] = (() => {
  function x() {}
  const wr = new WeakRef(x);
  function z() { return void x; }
  return [z, wr];
})();

Let's say that z lasts until the agent as a whole is truly dead. We can see that z does not actually use x. However, depending on the representation the compiler chooses, the runtime may not be able to know that z will not use x. Say z is used during the cleanup of something else, which would happen on agent exit. At what point can we say that the agent has exited "enough" that x will no longer be used, so we can do cleanup within the agent prior to the agent exiting? I'm sure the answer is that we cannot do so reliably.

We can only cleanup an agent death reliably from outside the agent, after the agent itself is already dead.

@littledan
Copy link
Member

I think we have a solid answer to this question ("no"), so closing the issue.

@wingo
Copy link
Collaborator

wingo commented Sep 27, 2019

FWIW I understand that the general pattern here is to use an auxiliary cleanup set.

Assuming all holdings are unique, when you register an external resource with a FinalizationGroup, you also add the resource's holding to an agent-global cleanup set.

When the FinalizationGroup's callback runs, it cleans up the resource and removes the holding from the cleanup set.

When the process exits, you explicitly clean up any remaining holdings in the cleanup set.

So, IIUC Node can implement agent-death cleanup itself.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Oct 13, 2019
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
gabrielschulhof pushed a commit to nodejs/node that referenced this issue Oct 13, 2019
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: #28428
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Oct 22, 2019
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: nodejs#28428
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 8, 2019
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: #28428
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Nov 10, 2019
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: #28428
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants