Skip to content
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

Make Ocaml global lock reentrant from the same thread #5299

Closed
vicuna opened this issue Jun 22, 2011 · 11 comments
Closed

Make Ocaml global lock reentrant from the same thread #5299

vicuna opened this issue Jun 22, 2011 · 11 comments

Comments

@vicuna
Copy link

vicuna commented Jun 22, 2011

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.

@vicuna
Copy link
Author

vicuna commented Jun 25, 2011

Comment author: @mmottl

I'd consider recursive lock acquisitions bad practice. There has never been a case in numerous complex bindings where I would have needed this feature. In mission-critical code I even prefer error-checking mutexes that prevent me from acquiring locks twice or releasing them once too often. As with everything multithreaded: the simpler the better. It's hard enough to reason about the simple case.

@vicuna
Copy link
Author

vicuna commented Jun 25, 2011

Comment author: @db4

OK, let's consider the COM binding (camlidl). A COM object (implemented in Ocaml) can be released explicitly via Release() or implicitly via its finalizer that in turn calls Release(). Release() is a C function that needs Ocaml runtime:

ULONG STDMETHODCALLTYPE camlidl_Release(struct camlidl_intf * this)
{
struct camlidl_component * comp = this->comp;
ULONG newrefcount = InterlockedDecrement(&(comp->refcount));

caml_leave_blocking_section();
/.../
caml_enter_blocking_section();
return newrefcount;
}

Now the finalizer goes:

static void camlidl_finalize_interface(value intf)
{
/* Ocaml lock is already acquired here! */
interface IUnknown * i = (interface IUnknown ) Field(intf, 1);
i->lpVtbl->Release(i); /
points to camlidl_Release() or another C function */
}

and you have a deadlock (Of course you cannot release Ocaml lock inside the finalizer before calling Release()). How to implement this without recursive locking?

Another problem is that you cannot make Win32 critical section non-recursive.

@vicuna
Copy link
Author

vicuna commented Jun 25, 2011

Comment author: @mmottl

I think it is generally a bad idea to call potentially blocking functions from within finalizers: one has practically no control over which thread executes a finalizer. This may have bad consequences on the behavior of the runtime, e.g. blocking a thread that needs to handle I/O. Even the mere unpredictability of when the finalizers are run may cause issues, e.g. exhausting resources like file descriptors managed by the C-data, etc.

@vicuna
Copy link
Author

vicuna commented Jun 25, 2011

Comment author: @db4

What do you mean by "potentially blocking function"? Release() actually only calls caml_remove_global_root() inside (no OS handle manipulation or whatever). And it can be called from outside so caml_leave_blocking_section()/caml_enter_blocking_section() wrapper is a must. Do you have any idea how to implement this logic without recursive locking?

@vicuna
Copy link
Author

vicuna commented Jun 27, 2011

Comment author: @mmottl

There was no code in the part of the "Release" function outside of the OCaml lock so I assumed that this could happen.

Anyway, at least in the above example it seems you can organize your code such that the finalizer cleans up after itself in a way that does not acquire the OCaml runtime lock a second time.

@vicuna
Copy link
Author

vicuna commented Jun 27, 2011

Comment author: @db4

Unfortunately I cannot do that easily. I see no way to tell Release() if it's called from a C function (and thus the lock is needed) or from a finalizer (and then the lock is not allowed). Release() specs are part of Microsoft COM API and cannot be changed. Recursive lock seems to be natural solution here. Why are you against it? If it's bad practice from your point of view, just don't use it (the feature may even be left undocumented). It breaks no existing code, but sometimes it's really helpful. And it can be added for free.

@vicuna
Copy link
Author

vicuna commented Jun 27, 2011

Comment author: @mmottl

I don't know much about how camlidl generates code in that case, but you can always choose to manually override or change it. And recursive locks do not come for free, there is not insignificant added overhead.

@vicuna
Copy link
Author

vicuna commented Jun 27, 2011

Comment author: @db4

Believe me, it cannot be done easily. E.g. because you should be able to Release()/finalize both Caml and non-Caml COM objects. Well, TLS-based solution would work but it's even worse practice (IMHO). As for the cost, under Windows it's absolutely free - Win32 critical section is already reentrant. Are POSIX recursive mutexes so bad that the overhead is significant?

@vicuna
Copy link
Author

vicuna commented Jun 27, 2011

Comment author: @mmottl

Yes, there is some overhead with recursive mutexes, e.g.: http://askldjd.wordpress.com/2009/10/26/prefer-simple-mutex-over-recursive-mutex

@vicuna
Copy link
Author

vicuna commented Jun 27, 2011

Comment author: @db4

Thanks for the info. BTW, if mutex overhead is really important (having only one mutex in Ocaml global lock I still doubt that), user-space mutex (futex) would be viable choice.

@vicuna
Copy link
Author

vicuna commented Mar 26, 2012

Comment author: @xavierleroy

Several OCaml developers just discussed this issue. We are split over it, but a majority is against recursive masterlock for the reasons listed by Markus Mottl. Feel free to add comments if other usage scenarios appear.

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

No branches or pull requests

1 participant