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

Document prior art / common finalizer pitfalls #78

Closed
wingo opened this issue Apr 8, 2019 · 11 comments · Fixed by #95
Closed

Document prior art / common finalizer pitfalls #78

wingo opened this issue Apr 8, 2019 · 11 comments · Fixed by #95

Comments

@wingo
Copy link
Collaborator

wingo commented Apr 8, 2019

I think it may be a good idea to mention the historical pitfalls that finalizers have had in practice from other languages, and how this proposal is affected by them. Just off the top of my head:

Reachable finalized objects

In Java, finalizers run as instance methods on the object being finalized. If a clique of finalizable objects becomes unreachable, Java is still able to collect the clique, but it may be that a finalizer for object A could see object B in an already-finalized state, which is often a source of bugs.

This proposal avoids this problem by design: objects are already dead and unreachable when finalizers run. Finalizers will never see a finalized object.

Scarce resource management

Scarce resources like file descriptors are often a motivation for finalizers, and indeed they show up in the explainer. The explainer is correct that finalizers are useful as a backstop to release scarce resources. However in practice it's possible that users write code that acquires scarce resources faster than the garbage collector will run; e.g.

for (const name of fileNames) {
  foo(openFile(name));
}

Here it's likely that the GC won't run before all file descriptors are exhausted, if fileNames is long enough.

Of course the correct construct here is something that will release scarce resources (file descriptors, in this case) promptly, i.e.

for (const name of fileNames) {
  let fd = openFile(name);
  try { foo(fd); } finally { closeFile(fd); }
}

However users do all kinds of things, and to be fair sometimes you don't know when you have this situation in testing and instead you see it in production. Although this spec doesn't create this hazard, as host objects may have finalizers of some sort, it will likely exacerbate it.

To compensate, in languages that expose an interface to a garbage collector, it's usual for the implementation of openFile to explicitly run a gc() if allocating a file descriptor fails. I think this spec is going to encourage users to ask for a standardized interface to run GC; it would be nice to avoid this pressure but I don't see a way around it (the pressure, I mean; we can probably avoid giving in, though).

Concurrent finalization

Finalizers introduce concurrency: in addition to the promises, callbacks, event handlers, and so on which are present in a JS system, there will also be finalizers. The only useful thing a finalizer can do is to modify global state in some way, so finalizers are necessarily concurrent with main-program modifications on that global state.

This proposal avoids two problems that Java experienced. One is that because finalization group callbacks are only invoked in their own microtask, they aren't concurrent with any other microtask. In contrast, in early Java they could be invoked within other "microtasks", from the "main thread". Early Java moved away from this model because if a finalizer needs lock A but the finalizer runs when the main thread already has aquired lock A, you get a deadlock. So Java moved to always run finalizers from dedicated threads, to avoid this issue.

The other problem Java still has but which this proposal does not have is that it runs finalizer "microtasks" from a separate thread, so they need judicious lock discipline to coordinate access to shared global state. This proposal doesn't need locks as such.

However there is the possibility for concurrent mutation during long-running async tasks. For example, consider:

  for (const fd of File.allFds()) {
    yield visit(fd);
  }

Here while the values from File.allFds() are being consumed, the finalizer may be mutating the set of values that are in that backing set. However this problem is shared with other kinds of shared-state concurrency in JS and the specifications cover what happens when e.g. a Set is modified while it is being iterated, so there's no particular hazard here.

Early finalization

Consider:

async function f(x) {
  let fd = x.fd;
  // yield some things
  g(fd);
}

When g is called, it could be that GC determined that x was no longer reachable, and that therefore its finalizer could be called. For synchronous functions, this proposal avoids the issue, as finalizers aren't concurrent with microtasks. However for async functions it's perfectly possible. Implementations have their own characteristics but I can imagine that they will push back on any specification of what it means for an object to be reachable.

The situation is of course aggravated in the presence of aggressive inlining and/or tail calls.

I am not sure that we can avoid this one entirely from the specification perspective and I think it's likely that some users will be bitten by this class of bug.

See also, "Finalization Should Not Be Based On Reachability", a small polemic by Hans Boehm: https://www.hboehm.info/misc_slides/ISMMfinalization.pdf, or also https://www.hboehm.info/popl03/web/html/slide_12.html.

Miscellaneous references

[1] Destructors, Finalizers, and Synchronization, Hans Boehm, https://www.hpl.hp.com/techreports/2002/HPL-2002-335.pdf

@wingo
Copy link
Collaborator Author

wingo commented Apr 8, 2019

Another one:

Finalizers may never be called

Sometimes users expect that a finalizer will always be called as soon as an object becomes unreachable (though what that means precisely is difficult to define). However we know this isn't true; not only is it possible for the finalizer to be called long after its target becomes unreachable, it's also possible that a finalizer is never called. I mention this because we should make special emphasis in explainers and so on that finalizers are probably not called when navigating away from a page, or when a node process exits, which I have found is contrary to user expectations. I think the only remedy is documentation.

@wingo
Copy link
Collaborator Author

wingo commented Apr 8, 2019

I think there are two audiences that could use documentation of this kind:

  • Developers should be aware of the limitations of finalizers. They are weirder than most people think

  • Other VM/language design people would probably appreciate that this spec took this issues into account. I know I was pleasantly surprised when I realized that this proposal avoids many issues that other languages have.

@wingo
Copy link
Collaborator Author

wingo commented Apr 8, 2019

Apologies for this segmented writing exercise :) One final thing. This is from the Guile manual:

Finalizers are tricky business and it is best to avoid them. They can be invoked at unexpected times, or not at all—for example, they are not invoked on process exit. They don’t help the garbage collector do its job; in fact, they are a hindrance. Furthermore, they perturb the garbage collector’s internal accounting. The GC decides to scan the heap when it thinks that it is necessary, after some amount of allocation. Finalizable objects almost always represent an amount of allocation that is invisible to the garbage collector. The effect can be that the actual resource usage of a system with finalizable objects is higher than what the GC thinks it should be.

I know that implementation-specific info is out of place in this proposal, but when it comes to design patterns for end users, a good understanding of the sharp edges does seem useful.

@littledan
Copy link
Member

This looks great. All of that looks like a good idea to add to the explainer in a PR. I wouldn't even mind if we cribbed that paragraph from Guile and put it way at the top (substituting "process exit" from "they might not be invoked when a tab is closed"). You just might need some massaging of the wording, e.g., at the end of "Early finalization", simply providing the rationale for giving implementations that freedom rather than blaming implementers for adding the constraint.

Would you be interested in making a PR to add this text towards the beginning of the article?

@wingo
Copy link
Collaborator Author

wingo commented Apr 8, 2019

Thanks for feedback. Any additional feedback also welcome. I will work on a PR later this week.

@wingo
Copy link
Collaborator Author

wingo commented Apr 23, 2019

I'll update documentation this week. In the meantime I discovered another pitfall: if you have a turn that allocates many finalizable objects, you end up with much chunkier memory usage. E.g. imagine that in a turn, you allocate 160000 finalizable objects. Even if GC runs, it won't finalize them until later, so at least you keep as many callback entries but possible also objects passed as the "holder" parameter. GC can't collect them eagerly as finalization waits for the next microtask, which might even lead to premature promotion and related things. Dunno if it needs documenting but I mention it here because it's a nonobvious result for me.

@littledan
Copy link
Member

@wingo Memory usage for the "holdings" seems like a serious concern to me. It's why I'd like to switch from the microtask checkpoint to any arbitrary task time for collection (so holdings can be freed while JS is idle for a while), but that will remain chunky as well.

@RichieAHB
Copy link

Potentially an obvious one here but the Problems section on Wikipedia is commonly linked to when mentioning issues with finalizers. I think it covers all of the points mentioned here (albeit less specifically) and a few others, it would be a good place to start when documenting them. E.g. exception handling errors in finalizers, no guaranteed execution order, object resurrection etc.

@wingo
Copy link
Collaborator Author

wingo commented Apr 24, 2019

I think my overview covered all points in the wikipedia article, i.e. explicitly mentioning that most don't apply directly as JS has no threads, finalizers run post-mortem, etc.

@erights
Copy link
Contributor

erights commented Apr 24, 2019

Hi @littledan I don't understand

It's why I'd like to switch from the microtask checkpoint to any arbitrary task time for collection (so holdings can be freed while JS is idle for a while), but that will remain chunky as well.

Could you expand? Thanks.

@littledan
Copy link
Member

@erights #61 (comment)

wingo added a commit to wingo/proposal-weakrefs that referenced this issue May 9, 2019
littledan pushed a commit that referenced this issue May 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants