cc @Engil and @kayceesrk (who worked on this aspect of the code)

One aspect where this PR is lacking is on the testing front: I haven't tried to reproduce the CI failure on my machine (and I doubt it would work as-is), so I can't tell whether this really was the issue and whether this PR solves it. (In theory this was and it does, but this is the theory.)

Given that apparently this bug was already noticed during multicore-ocaml development ( #11800 (comment) ), it would be nice to think of a reliable way to have a regression test. Any suggestions?

(One experiment I can make is to introduce a large pause at the end of domain termination, after domain_self is made available for reuse, and see whether the issue triggers more easily. If it works, maybe we could introduce such a pause in debug mode, just to make this category of domain-reuse issues trigger more often?)

(One experiment I can make is to introduce a large pause at the end of domain termination, after domain_self is made available for reuse, and see whether the issue triggers more easily. If it works, maybe we could introduce such a pause in debug mode, just to make this category of domain-reuse issues trigger more often?)

I think this is quite a fun idea, we could introduce such delays in the debug runtime, and it could perhaps make it easier to spot lifecycle regressions with tight timing triggers?

I tried to add a pause in various places in domain_terminate:

  caml_plat_spin_wait(
      10*1000*1000 /* 10ms */,
      __FILE__, __LINE__, __func__);

but I didn't get any luck triggering the bug after that -- I tried to run the tests/parallel tests with the pause and the debug runtime enabled, and that was not enough (or maybe I did things wrong). That's all from me tonight :-)

Looking at the CI, I feel like the fix is not quite it, there's something more subtle to be taken into account and I was wrong in my assumption that it was probably ok to move BT termination here.

On the parallel/join.ml test, I can see the following behavior:
Thread 2 attempt to terminate, trigger a minor cycle.

Thread 2 received signal SIGSEGV, Segmentation fault.
caml_empty_minor_heap_promote (domain=domain@entry=0x3e5024002b70, participating_count=participating_count@entry=3, participating=participating@entry=0x4da9a0 <stw_request+64>) at runtime/minor_gc.c:511
511           intnat major_ref_size = foreign_major_ref->ptr - foreign_major_ref->base;

The foreign_domain->minor_tables being accessed is one of a recently terminated domain, as we can see by reversed stepping to it being set to 0.

Thread 8 hit Hardware watchpoint 2: *0x3e5024002bc8

Old value = 0
New value = 738207968
domain_terminate () at runtime/domain.c:1745
1745      domain_state->minor_tables = 0;
(rr) bt
#0  domain_terminate () at runtime/domain.c:1745
#1  domain_thread_func (v=<optimized out>) at runtime/domain.c:1094
#2  0x0000000000296822 in start_thread (arg=<optimized out>) at pthread_create.c:443
#3  0x0000000000236314 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:100

I feel like we now may be release the backup thread too early, and it is either bad, or there's another bug lurking around, but it is bed time for me.

@kayceesrk I agree that releasing the domain lock is quite probably the cause of the testsuite failure here. (But then I tried to understand more precisely the protocol around the domain lock and I got a bit lost.)

Let me try to explain what is bugging me about domain_lock, maybe the experts in the room can let me understand better. If I read the code correctly, the part of domain_terminate that writes the BT_TERMINATE status is done while holding the domain lock, and releases the domain lock right away:

On the other hand, create_domain takes the domain lock:

and the call to install_backup_thread only occurs after this domain_create call has taken the lock:

Naively this suggests that the race scenario I described above, when creating this PR, cannot actually happen: a new domain reusing the same dom_internal slot could only install its backup thread after the previous domain termination has released the domain lock, and this occurs after BT_TERMINATE has been written, so at this point the install_backup_thread assertion failure is not possible anymore.

But: there are other uses of the domain lock, and it may be that something funny happens. (Could something in domain_terminate release and re-acquire the domain lock after leaving the STW set but before writing the BT_TERMINATE status?)

I must say that it's not so easy to follow the use of the various locking mechanisms in domain.c and the invariants they should protect. For example the backup thread sometimes waits on an interruptor-related condition variable s->cond, sometimes on domain_cond, and it is not obvious why or what assumptions are made. Independently, the part of domain_terminate after removal from the STW set seems careful not to access domain_self, only domain_state... except precisely for the three lines that deal with the backup thread.

Side question: online documentation on condition variable suggests to use a while (cond-is-false) pthread_cond_wait(cond) approach, to avoid spurious wakeups. I wonder why the backup thread code uses a simple if (cond is false) pthread_cond_wait(cond) instead, what happens in the case of spurious wakeups?

(I'm at the point where I realize that I don't understand much of what is going on here, and I am thinking of closing the PR to go back to the issue discussing the assert failure.)

(I'm at the point where I realize that I don't understand much of what is going on here, and I am thinking of closing the PR to go back to the issue discussing the assert failure.)

I am happy to take a look at clarifying/reorganizing the runtime code around domain termination for readability. This can be done elsewhere. For this issue, I do think that we should understand and reproduce the assertion failure before we can propose a solution. So closing this PR is the right thing to do.