Original bug ID: 5299
Reporter: @db4
Status: closed (set by @xavierleroy on 2015-12-11T18:04:24Z)
Resolution: won't fix
Priority: normal
Severity: feature
Version: 3.12.0
Category: ~DO NOT USE (was: OCaml general)
Monitored by: mehdi @ygrek

Bug description

Suppose you have several functions that allow C side to call Ocaml callbacks. Your usually wrap their bodies in

leave_blocking_section();
enter_blocking_section();

Now one such function can call another one inside the wrapped area:

void function1()
{
leave_blocking_section();
caml_callback(callback1, Val_unit);
enter_blocking_section();
}

void function2()
{
leave_blocking_section();
function1();
caml_callback(callback2, Val_unit);
enter_blocking_section();
}

and you are in trouble because you have nested

leave_blocking_section();
leave_blocking_section();
enter_blocking_section();
enter_blocking_section();

The current systhreads implementation deadlocks under Posix, but surprisingly pretends to work under Win32. I use word "pretend" because it does not block thanks to Win32 CRITICAL_SECTION being reentrant, but local roots allocated between 1st and 2nd leave_blocking_section() are lost and you'll see the crash soon.

Why not to modify systhreads implementation and officially allow recursive lock acquire from the same thread? For Win32 it's trivial:

typedef struct {
CRITICAL_SECTION lock;
int recursion_count;
} st_masterlock;

static INLINE void st_masterlock_acquire(st_masterlock * m)
{
TRACE("st_masterlock_acquire");
EnterCriticalSection(&m->lock);
++(m->recursion_count);
TRACE("st_masterlock_acquire (done)");
}

static INLINE void st_masterlock_release(st_masterlock * m)
{
--(m->recursion_count);
LeaveCriticalSection(&m->lock);
TRACE("st_masterlock_released");
}

static void caml_thread_enter_blocking_section(void)
{
if (caml_master_lock.recursion_count == 1) {
/* ... /
}
/ Tell other threads that the runtime is free */
st_masterlock_release(&caml_master_lock);
}

static void caml_thread_leave_blocking_section(void)
{
/* Wait until the runtime is free /
st_masterlock_acquire(&caml_master_lock);
if (caml_master_lock.recursion_count == 1) {
/ ... */
}
}

For POSIX you just should use PTHREAD_MUTEX_RECURSIVE lock and correctly handle condition variable.