New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid finalizer rerun unless object is rescued #473

Merged
merged 5 commits into from Dec 3, 2015

Conversation

Projects
None yet
2 participants
@svaarala
Copy link
Owner

svaarala commented Nov 30, 2015

Change duk_hobject_run_finalizer() so that:

  • If DUK_HEAPHDR_FLAG_FINALIZED is already set, do nothing.
  • Set DUK_HEAPHDR_FLAG_FINALIZED just before running finalizer.

This should guarantee the semantics described in #109 (= at most one finalizer call per rescue cycle) because:

  • The only place where a finalizer is called is duk_hobject_run_finalizer().
  • The FINALIZED flag is only cleared by an explicit rescue, detected by either mark-and-sweep or refcounting.

Other changes:

  • Heap destruction finalizer behavior changes: allow finalizers to create new finalizable objects whose finalizers are also executed, sanity limit.
  • Explicit Proxy check, don't call finalizer on a Proxy object.

Tasks:

  • Review changes
  • Try to come up with a repro case for finalizer re-run, most obvious cases are already fixed
  • Test case for runaway finalizer which spawns 1 or 2 new objects per finalization
  • Test case for new finalizer argument
  • Ensure "mark-and-sweep running" is checked even when mark-and-sweep not enabled
  • Website changes for new guarantee
  • Internal doc changes for new guarantee
  • Add second parameter to finalizer indicating if rescue is possible or not (true: heap being destroyed, can't rescue; false: normal rescue possible)
  • Finalizer documentation changes for new 2nd argument
  • Run heap destruction finalizers in a loop to allow any new finalizable objects created by previous finalizers to also be finalized
  • Add a sanity limit to the finalization loop
  • Releases
  • Assert run with mark-and-sweep + refcounting
  • Assert run with mark-and-sweep only
  • Assert run with refcounting only

Fixes #109. See #108.

@svaarala svaarala added this to the v1.4.0 milestone Nov 30, 2015

@svaarala svaarala force-pushed the avoid-finalizer-rerun branch 4 times, most recently from c0edc6c to a094b13 Nov 30, 2015

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 1, 2015

A question just occurred to me: What happens if a finalizer rescues an object during heap destruction, say by putting it into the global stash? Will this create an infinite loop or perhaps a memory leak? Or will it be handled gracefully?

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

It won't cause any trouble - the finalizer will be executed once, but not twice.

@svaarala svaarala force-pushed the avoid-finalizer-rerun branch 2 times, most recently from 5871129 to 13fb973 Dec 1, 2015

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

There was a potential issue in the heap destruction finalizer handling: if a refzero situation occurred an object might be queued into the refzero list which was not processed.

Fixed this so that the heap destruction code fakes a "mark-and-sweep running" flag which prevents a mark-and-sweep and also prevents refzero from moving objects around in the heap linked lists (in particular, it prevents an object from being moved from heap_allocated to refzero_list).

This change also means "mark-and-sweep running" flag needs to be checked for in refzero handling even when mark-and-sweep itself is disabled.

@svaarala svaarala force-pushed the avoid-finalizer-rerun branch 3 times, most recently from e29127d to 825c9c5 Dec 1, 2015

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 1, 2015

So that's a potential memory/resource leak for the application then, if the finalizer attempts to rescue the object (instead of freeing the resource) but then doesn't get called again because we're in heap destruction... Might be worth noting that in the documentation.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

Agree, I'll describe the limitations for finalization during heap destruction on the website.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

Somewhat related - IIRC right now any new garbage/objects created during heap destruction won't get their finalizers executed.

That's useful in the sense that heap destruction won't get stuck if a finalizer always creates one or more new finalizable objects. On the other hand it violates the base finalization policy for these new objects. And, there are legitimate reasons to create finalizable garbage in a finalizer, and if the application is correctly written that process will eventually terminate.

I could change the heap destruction so that it'll walk through the heap_allocated list running finalizers until there are no more finalizers to run (or perhaps some sanity limit is reached, say 10 passes over the heap?). This would have the upside that new garbage would get their finalizer executed (which may be useful if only a limited amount of garbage is created); the downside would be the potential to get forever stuck destroying a heap. There's potential for that anyway though: a finalizer may simply block, for example.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

Heh, I think I'm starting to remember why I didn't clean this up before... There's a lot of corner cases to consider, and the resulting behavior should be both reasonable (users probably expect heap destruction to succeed most of the time) and intuitive (finalizers are executed).

I'll try out a few alternatives to see what they look like; I'll update the website text and testcases to match.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

The current limitations in heap destruction would be:

  • A finalizer is only executed once if the object is rescued.
  • If a finalizer creates new objects with finalizers, they will be prepended to heap_allocated and their finalizers won't run.

These can be tweaked in various ways, for example:

  • Rescuing can be supported by simply looping finalization and forced mark-and-sweep until no finalizable objects remain. Add a sanity limit if it's useful to guarantee the process terminates forcibly; this will necessarily mean some finalizers won't run, and have an associated potential leak.
  • New objects created by a finalizer can also be finalized simply by looping the heap_allocated list multiple times until no finalizable objects remain. A sanity limit can be applied to this too, with the same caveat that some finalizers won't run.

The main question is what guarantee is the most intuitive, given that there's a tradeoff in ensuring heap destruction eventually finishes vs. skipping some finalizers after a certain sanity round limit.

There's also a limitation when script timeout is active and the timeout RangeError is propagating:

  • When the timeout error is propagating, any Ecmascript finalizers will start executing but will RangeError immediately. If this is a concern, finalizers should be Duktape/C functions.

This limitation could be mitigated by flagging the finalizer call so that the script timeout error wouldn't be thrown while the finalizer runs. This would be nice, but I'm not sure if this can be done cleanly.

Regardless, this needs to be described as a finalizer guarantee limitation (I added it to the website already in this pull, but I'll rewrite the section to be a bit easier to read with subsections).

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 1, 2015

I'm thinking the "infinite loop if a finalizer creates new finalizable garbage" behavior might not be that "surprising" - see #310.

In the longer term, for sandboxing reasons we may still want to have some kind of timeout mechanism in place, however.

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 1, 2015

It would be interesting I think to find out how other environments, e.g. .NET, handle runaway finalization cases.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

Regarding #310, I really think that behavior is really just like any other resource consumption bug, similar to an application just creating reachable garbage in a busy loop or something. There's not really much to do about those cases. I can write an application which exhausts memory quite easily and there's no general fix for that.

However, things are perhaps a bit different when you're trying to destroy the heap. It might be preferable to be able to destroy the heap (even at the cost of not running some finalizers) rather than hang in such a case. It'd be nice, for example, to be able to use a script timeout to ensure finalizers finish in a reasonable time, even when no script timeout is used in the normal execution mode.

But I guess I'm leaning on not making an exception in the heap destruction: just run finalizers until they're all done. If the application has a bug which creates garbage in an infinite loop, it's a bug that would most likely affect the program in normal operation too.

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 1, 2015

Okay, found this (.NET finalization behavior):
https://msdn.microsoft.com/en-us/library/system.object.finalize(v=vs.110).aspx#How

In short: .NET guarantees the finalizer will only be called once, and calls ALL finalizers on shutdown, regardless of accessibility. This bit is interesting though:

The runtime continues to finalize objects during shutdown only while the number of finalizable objects continues to decrease.

Which implies that if the "number of finalizable objects" increases instead, it will bail out of finalization? Hm.

Also, this:

Finalize can take almost any action, including resurrecting an object (that is, making the object accessible again) after it has been cleaned up during garbage collection. However, the object can only be resurrected once; Finalize cannot be called on resurrected objects during garbage collection. There is one action that your implementation of Finalize should never take: it should never throw an exception.

Rescued objects are not re-finalized (essentially, they are zombies).

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

Yeah, I was thinking about something like that earlier today. For example:

  • When heap destruction starts, run a single round of finalization. Count the number of finalizers called as n_initial.
  • Set n_limit as some multiple of that, e.g. 4 * n_initial.
  • Run finalization rounds; keep on running as long as finalizers executed is below n_limit. Decrease n_limit on every round to ensure termination.

But I thought it was maybe a bit too complicated. Perhaps not? :)

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 1, 2015

Despite what I said above, it definitely makes sense to guarantee termination on heap destruction--I think we all know how annoying it is to hit the close box on a window and have it take an age and a day to actually terminate (assuming it ever does). In most cases--on desktop platforms at least--any leaks will be cleaned up by the OS anyway.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

One approach would be to run the finalizer rounds but call user a callback between rounds and let it decide whether or not to keep running or abort and ditch the remaining finalizers. This would require either an API change, a new API call, or it might be implemented as a config macro like script timeout.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

Considering the kinds of applications where this matters: if there's a runaway finalizer, then there isn't a clean solution: either the heap destruction hangs or some finalizers won't run. But it doesn't maybe really matter if the source is an application bug.

Intended behavior where a finalizer might spawn new finalizable objects definitely exist. For example, you might be finalizing an object which requires a HTTP request to inform a server of a state change. That server request may have native state which needs finalization. Perhaps that HTTP request finalizer will use another resource which needs finalization. But in the end this chain of finalizers would normally terminate pretty quickly.

So to allow this to happen, the barebones solution where at most N finalization rounds are executed (say N=10) should mostly work. It will work badly if the number of finalizable resources doubles on every round; the number of finalizable garbage would go up 1000x before the sanity kicked in.

The .Net-like approach is a bit more refined, it doesn't impose a maximum round count, but keeps an eye on the application behaving badly. It'd also be quite easy to implemented roughly along the lines I pointed to above (get an initial limit based on first round and then start reducing that, e.g. by 20% on every round).

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 1, 2015

Duktape also has it a bit harder compared to .NET - rescued objects become eligible for finalization again. So it's possible to have an accidental runaway just from normal rescue logic. .NET avoids this issue by treating rescued objects as zombies.

I personally prefer the Duktape rescue behavior, just pointing out that it does make this problem more complicated. :)

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

Based on the discussion above I think the best approach so far is:

  • Support finalizer rescue during heap destruction: provide the same guarantee of "one call per rescue cycle" for rescued objects.
  • Allow finalizers to create more finalizable objects during heap destruction. Run their finalizers too.
  • Impose a sanity limit to detect runaway finalizers using a shrinking limit for number of finalizers executed per finalization round. This should be lenient enough to not affect ordinary programs.

This would provide "normal" finalizer guarantees for heap destruction, except in the case of runaway finalizers. That'd need to be described in the documentation too, with the specific algorithm maybe left a bit vague so that it can be finetuned without breaking an explicit guarantee.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 1, 2015

Regarding rescue behavior, at one point the behavior was to finalize only once. But for native resources that makes rescuing pointless. Native resources are of course not the only reason to rescue an object so that policy is still useful in some other situations.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 2, 2015

Hmm, I don't think so. The core of the problem is that any finalized objects with FINALIZED flag set must be processed before forced finalization, but we don't want to finalize further objects when doing so. "Processing" concretely means clearing the FINALIZED flag, and detecting if the object was rescued; if so, move it back to heap_allocated so that it gets finalized again (as required by the finalizer guarantees).

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 2, 2015

Anyway, the pushed solution should work (and does work for the test case) - it's pretty simple in the end: mark-and-sweep will just handle rescue normally, but any new finalizable objects will be just ignored and moved back to heap_allocated for forced finalization.

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 2, 2015

So just to clarify: if in the situation described above, the object isn't rescued during mark-and-sweep, the finalizer won't be called again, correct?

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 2, 2015

Yes, the finalizer guarantees will hold: the second argument will be false on the last normal GC run to indicate rescue is possible; if the object is not rescued, no more finalizer calls will happen. If the object is rescued, one more finalizer call will happen, this time with the finalizer second argument being true.

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 2, 2015

Ah, so it's just an extra rescue cycle, that's no problem then.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 2, 2015

I'd leave out the "extra" because the rescue is up to the finalizer :)

Finalizer guarantee: run once per rescue cycle
- Set FINALIZED only when actually running finalizer to prevent
  a finalizer running twice unless explicitly rescued (and the
  flag cleared by the rescuing mark-and-sweep or refzero code).

- Add a guard to avoid re-finalizing until FINALIZED is explicitly
  cleared on rescue by either mark-and-sweep or refcounting.

- Prevent mark-and-sweep and refzero from running when heap is
  destroyed and finalizers are forcibly executed.

- Add a mark-and-sweep flag to skip finalizers: move any finalizable
  objects back to heap_allocated.  This is needed for correct
  finalizer handling in heap destruction.

- Add second finalizer argument; if true, it indicates that the
  heap is being destroyed and rescuing an object is not possible.
  Finalizer should therefore free all native resources without
  relying on the finalizer to be called again.

- Add multiple finalizer rounds for heap destruction to deal with
  finalizers which create further finalizable objects.  Also add
  a sanity limit for this process to catch runaway finalizers.

- Explicit Proxy check just before running finalizer: don't run
  finalizer for Proxy objects even when call site is buggy.

Src fiexs

@svaarala svaarala force-pushed the avoid-finalizer-rerun branch 2 times, most recently from 1dad052 to 766c731 Dec 2, 2015

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 2, 2015

I think this is ready to be merged now - I'll merge in a bit.

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 2, 2015

Regarding the dangling heapptr pushed onto the stack in this test:
https://github.com/svaarala/duktape/blob/1e8499012adf43634ccc011aafb5d2b8bfce5bbc/tests/api/test-dev-finalizer-rerun.c

How does this not segfault the minute duk_push_heapptr() is called?

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 2, 2015

The object referenced by ptr is intentionally in a reference loop so it can't be collected by refcounting (even when it's not reachable). When duk_push_heapptr() happens, the object becomes reachable again and Duktape is none the wiser.

This is not a "supported" process: anything triggering a mark-and-sweep before duk_push_heapptr() leads to memory unsafe behavior. It works in the test case well enough to cover a case which is otherwise difficult (if not impossible) to reproduce.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 2, 2015

Sorry, there's an explicit GC in there before the duk_push_heapptr(), so it really is quite an unsafe test. It's the only way I found to exercise this particular finalizer re-entry case, but it's maybe too fragile to be in the test set; I'll disable it.

@svaarala svaarala force-pushed the avoid-finalizer-rerun branch from 766c731 to a9f9a4d Dec 2, 2015

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 2, 2015

In theory that actually should be safe now that I look at it again - it takes two GC cycles to collect an object with a finalizer, you're just tricking Duktape into making the already-finalized but unfreed object reachable again.

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 2, 2015

Yeah, that's what intended to happen. But there are quite a few assumptions in there so I disabled the test (but left the test code inside an #if 0 block for manual testing in case this ever turns up).

@svaarala svaarala force-pushed the avoid-finalizer-rerun branch from a9f9a4d to 934946c Dec 3, 2015

@svaarala svaarala force-pushed the avoid-finalizer-rerun branch from 934946c to f2e5378 Dec 3, 2015

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 3, 2015

Aiee!

svaarala added a commit that referenced this pull request Dec 3, 2015

Merge pull request #473 from svaarala/avoid-finalizer-rerun
Avoid finalizer rerun unless object is rescued

@svaarala svaarala merged commit f777103 into master Dec 3, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@svaarala svaarala deleted the avoid-finalizer-rerun branch Dec 3, 2015

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 3, 2015

What happened, forgot to merge the changes? :)

@svaarala

This comment has been minimized.

Copy link
Owner

svaarala commented Dec 3, 2015

Took a leap of faith that the merge improves finalizer behavior and doesn't introduce new corner cases :)

@fatcerberus

This comment has been minimized.

Copy link
Contributor

fatcerberus commented Dec 3, 2015

If nothing else it'd be interesting to see if this fixes #108.

@wenq1 wenq1 referenced this pull request Jun 24, 2017

Closed

Finalizers usage #1585

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment