tests/weak-ephe-final/finaliser_handover is flaky

[gasche]

This tests seems to fail from time to time on the github CI, for different reasons and on different systems. The test could be wrong, but it may also catch one or several bugs. In any case, those occasional failure are costing developer time -- it is tedious to lookup CI failures, and brain-consuming to make the guess that they are unrelated to the current PR.

• for Linear-time closure computation #12222, Allow make promote-menhir from Windows #12308, Update unix.mli #12338, Multicore statmemprof API #12381, Allow to name closed types in constructor type annotations #12507 the test failed on a linux machine (both bytecode and nativee failures where observed), with the error

  [02] file runtime/domain.c; line 1847 ### Assertion failed: caml_gc_phase == Phase_sweep_and_mark_main

• for Proper attribute propagation in presence of objects #12344 the test failed for native code on a macos machine, with the error

  Fatal error: exception File "finaliser_handover.ml", line 39, characters 2-8: Assertion failed

• for Propagate the classification from Rec_check #12551 and Remove ocamldoc's literate and todo generators #12615 the test failed in bytecode on a Windows machine

[kayceesrk]

I've assigned this issue to myself. The first assertion failure either indicates a bug or an incorrect assertion. I'll spend some time to investigate this.

[xavierleroy]

Why don't we disable this test until it is analyzed? A CI that fails randomly is useless.

[gasche]

If @kayceesrk happens to not have the time to look at this, maybe we could consider asking @fabbing or @NickBarnes for help?

@xavierleroy why not indeed; my question above is in the mindset of getting the bug fixed, rather than just denoising the CI.

[gasche]

Today @Octachron and @damiendoligez and myself tried to look at this. We timed out for now, but here is what we understand of the issue.

The bug comes from the following code in domain.c:

static void handover_finalisers(caml_domain_state* domain_state)
{
  struct caml_final_info* f = domain_state->final_info;
  if (f->todo_head != NULL || f->first.size != 0 || f->last.size != 0) {
    /* have some final structures */
    if (caml_gc_phase != Phase_sweep_and_mark_main) {
      /* Force a major GC cycle to simplify constraints for
       * handing over finalisers. */
      caml_finish_major_cycle();
      CAMLassert(caml_gc_phase == Phase_sweep_and_mark_main);
    }
    caml_add_orphaned_finalisers (f);
    /* Create a dummy final info */
    domain_state->final_info = caml_alloc_final_info();
  }
  caml_final_domain_terminate(domain_state);
}

The "handover" function is responsible, on domain termination, for adding the finalizers of the current (terminating) domain to a global list of "orphaned" finalizers.

The logic in this code is as follows:

• we only want to perform the handover if we are in a specific GC phase: sweep_and_mark_main (why? the code says "to simplify constraints", but what does this mean? documentation here would be welcome)
• if we are not in this phase, force to finish the current major GC to get back in this phase (caml_finish_major_cycle();)

The assertion that we are then in the desired phase fails.

What we believe happens is that finishing the current major GC cycle puts us in the correct phase, but some other domain makes progress in parallel and, by the time we get to the assert (or to the orphaning outside debug mode) we are not in this phase anymore.

It is not obvious how to fix this bug. An idea would be to delegate the orphaning work to the finish_major_cycle code: domains can put themselves in the state "I need to handover my finalizers", and the major-GC logic could call add_orphaned_finalizerrs at the right time, when we are still in the stop-the-world phase and the major GC cannot make concurrent progress.

One issue with this proposal is that it requires forcing a major GC cycle at each domain termination: we cannot rely on the if (caml_gc_phase != Phase_sweep_and_mark_main) test (as we believe it is not robust to concurrent GC work), we have to call finish_major_cycle unconditionally.

It would be nice to better understand the constraints on finalizers / when the handover can happen. What are the reasons why we want to be in a specific GC phase? cc @kayceesrk

(I thought at first that the constraint tries to avoid a race on the global list of orphaned finalizers, but that list seems to be protected by a mutex anyway.)

[gasche]

I went back to this issue today and indirectly started #12591 -- which touches the same part of the codebase but fixes another issue, a data race on orph_structs.