Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0005299OCamlOCaml generalpublic2011-06-22 18:072012-03-26 15:47
Reporterdb 
Assigned To 
PrioritynormalSeverityfeatureReproducibilityalways
StatusresolvedResolutionwon't fix 
PlatformOSOS Version
Product Version3.12.0 
Target VersionFixed in Version 
Summary0005299: Make Ocaml global lock reentrant from the same thread
DescriptionSuppose 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.
TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0006022)
mottl (reporter)
2011-06-25 16:54

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.
(0006023)
db (reporter)
2011-06-25 18:31

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.
(0006024)
mottl (reporter)
2011-06-25 19:53

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.
(0006025)
db (reporter)
2011-06-25 20:25

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?
(0006026)
mottl (reporter)
2011-06-27 06:06

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.
(0006029)
db (reporter)
2011-06-27 14:53

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.
(0006030)
mottl (reporter)
2011-06-27 15:01

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.
(0006031)
db (reporter)
2011-06-27 15:31

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?
(0006033)
mottl (reporter)
2011-06-27 17:22

Yes, there is some overhead with recursive mutexes, e.g.: http://askldjd.wordpress.com/2009/10/26/prefer-simple-mutex-over-recursive-mutex [^]
(0006034)
db (reporter)
2011-06-27 18:43

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.
(0007166)
xleroy (administrator)
2012-03-26 15:47

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.

- Issue History
Date Modified Username Field Change
2011-06-22 18:07 db New Issue
2011-06-25 16:54 mottl Note Added: 0006022
2011-06-25 18:31 db Note Added: 0006023
2011-06-25 19:53 mottl Note Added: 0006024
2011-06-25 20:25 db Note Added: 0006025
2011-06-27 06:06 mottl Note Added: 0006026
2011-06-27 14:53 db Note Added: 0006029
2011-06-27 15:01 mottl Note Added: 0006030
2011-06-27 15:31 db Note Added: 0006031
2011-06-27 17:22 mottl Note Added: 0006033
2011-06-27 18:43 db Note Added: 0006034
2012-03-26 15:30 lefessan Status new => acknowledged
2012-03-26 15:47 xleroy Note Added: 0007166
2012-03-26 15:47 xleroy Status acknowledged => resolved
2012-03-26 15:47 xleroy Resolution open => won't fix


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker