Fix #11865 by detecting deletion of a root during root scanning.

[stedolan]

If a C finaliser for a custom object tries to unregister a root with caml_remove_global_root or caml_remove_generational_global_root, then the runtime currently deadlocks (or otherwise goes wrong, platform-dependent) by trying to recursively acquire a nonrecursive mutex.

Changing this to a recursive mutex would not help: even if we got past the locking code, deleting a root while iterating would then corrupt the skiplist. The problem is that a data structure (the roots skiplist) is being modified during iteration.

The fix is straightforward: instead of removing roots during iteration, mark them to be deleted lazily. There's also a test, which deadlocks before this patch and succeeds after.

FAQ (foreseeably asked questions):

• Why use a thread-local variable to detect iteration?
  If a root is deleted while another thread is iterating, there is no problem: it simply waits for the lock as usual, and the iteration will be complete before the root is deleted. The problem only arises when the current thread is the one iterating.

• Why not always use lazy deletion, instead of detecting when an iteration is occurring?
  It is possible for a large number of roots to be created and removed within the same GC cycle. Lazy deletion while no root scan is in progress would cause the size of the skiplist to grow without bound.

• Doesn't lazy deletion make the skiplist bigger in any case?
  Yes, but by a bounded amount. The maximum length of the skiplist with this patch is at most double the length before it, since lazily deleted nodes are (a) only created during root scanning and (b) deleted during either the same root scanning cycle or the next one.

• What if a new root is added with the same address as one being lazily deleted?
  This works fine: caml_skiplist_insert overwrites the current entry if the key is already present, so the lazy deletion node will be overwritten with a normal root.

[jberdine]

Thanks, it would be great to see the return of the ability to unregister roots from finalizers. Minor nit, it would be good to partially roll back the manual change from #11876 that states that global roots must not be (un)registered from finalizers.

[mshinwell]

A couple of comments, but this looks correct to me.

[damiendoligez]

Looks good modulo two small remarks. Thanks for finding a simple solution to this hairy problem. Don't forget to update the manual (line 2034 of manual/src/cmds/intf-c.etex).

[stedolan]

I've updated the manual accordingly.

[gasche]

I was looking at this change again in the context of #13732 and was confused: it's not clear what exactly leads to a deadlock between skiplist traversal and skiplist element removal in the handling of custom block finalizers. In particular I expected the issue to come from finalizing a custom block on the minor heap (because those are typically more trouble), but minor custom blocks are not finalized during any kind of root traversal, as far as I can see, they are finalized in custom_finalize_minor which a distinct phase of the minor GC.

My current guess as to what is happening is that the finalizers causing trouble are those in the major heap. We are traversing the set of global generational young roots during a minor GC, we call oldify_* on the values reachable from those, which allocates into the major heap; the major heap works to find a place for the promoted value, and in the process it decide to do some sweeping work. At this point it can call a custom block finalizer, when sweeping the value.

This class of issues could possibly be avoided by storing custom blocks separately in the major heap, to be swept during major GC work only. The goal is not to increase the expressivity of custom block finalizers (they are still called during a minor GC, so fairly restricted in what is safe), but to relax the constraint that root-registration implementations must support reentrant removal in the middle of scanning. (Or is there another general reason why we should assume that root registration, modification, deletion can happen during root scanning?)

[stedolan]

My current guess as to what is happening is that the finalizers causing trouble are those in the major heap. We are traversing the set of global generational young roots during a minor GC, we call oldify_* on the values reachable from those, which allocates into the major heap; the major heap works to find a place for the promoted value, and in the process it decide to do some sweeping work. At this point it can call a custom block finalizer, when sweeping the value.

Yes, that sounds right to me.

This class of issues could possibly be avoided by storing custom blocks separately in the major heap, to be swept during major GC work only. The goal is not to increase the expressivity of custom block finalizers (they are still called during a minor GC, so fairly restricted in what is safe), but to relax the constraint that root-registration implementations must support reentrant removal in the middle of scanning. (Or is there another general reason why we should assume that root registration, modification, deletion can happen during root scanning?)

I don't understand the motivation here - lazy deletion is easy to implement (see this patch, but I struggle to imagine a root-registration scheme where it would be hard), and the constraint that root removal must never happen during scanning is tricky to enforce (see the bug this patch fixes, and its various other attempted fixes). Could you explain your goal here a bit more? (Also, when you say "class of issues", what are you thinking of beyond #11865?)