firefly icon indicating copy to clipboard operation
firefly copied to clipboard

`Process::acquire_heap` should be made re-entrant

Open hansihe opened this issue 6 years ago • 3 comments

The documentation for ProcessControlBlock::acquire_heap is reentrant. This code seems to use the rust std Mutex, which is not.

https://github.com/lumen/lumen/blob/aeae1721d0fc02982898c8bcd1e7ed0ae5db151c/liblumen_alloc/src/erts/process.rs#L202-L204

This causes issues when (for instance) using ProcessControlBlock::tuple_from_iter along with an iterator that allocates.

hansihe avatar Aug 21 '19 19:08 hansihe

Yeah, I saw the same behavior and mostly use the heap functions directly in process for that reason now.

KronicDeth avatar Aug 21 '19 20:08 KronicDeth

The reason is that the lock used to be a ReentrantMutex, not a Mutex; at some point that changed but that comment wasn't updated. If it needs to be re-entrant, we should make the necessary changes to support using the former, but if not, then we can just remove that note from the docs.

bitwalker avatar Sep 19 '19 08:09 bitwalker

I actually hit this during my work on #350, in that some tests were acquring a lock directly, then calling code which internally tried to acquire a lock. The result was that property tests would just hang for no apparent reason, in random places.

I think we need to either find a way to make this lock the Reentrant version again, or disallow acquiring the lock directly except in specific situations where we can guarantee re-entrancy isn't needed - otherwise it is going to be far too easy for code to slip in which introduces a deadlock.

bitwalker avatar Nov 19 '19 20:11 bitwalker

No longer relevant

bitwalker avatar Mar 11 '23 20:03 bitwalker