fixup! threads: avoid deadlock from recursive lock acquire

fix finalizers reset in jl_eh_restore_state

base/gcutils.jl

@@ -113,9 +113,6 @@ Increment or decrement the counter that controls the running of finalizers on
 the current thread. Finalizers will only run when the counter is at zero. (Set
 `true` for enabling, `false` for disabling). They may still run concurrently on
 another thread.
-
-When exiting any try block, either normally or due to an exception, this may be
-reset to the previous value from entering the try block.
 """
 enable_finalizers(on::Bool) = ccall(:jl_gc_enable_finalizers, Cvoid, (Ptr{Cvoid}, Int32,), C_NULL, on)
 

src/gc.c

@@ -410,7 +410,7 @@ JL_DLLEXPORT void jl_gc_enable_finalizers(jl_ptls_t ptls, int on)
             jl_error(""); // get a backtrace
         }
         JL_CATCH {
-            jl_printf((JL_STREAM*)STDERR_FILENO, "WARNING: GC finalizers already enabled on this thread. Possibly unpaired within a try block?\n");
+            jl_printf((JL_STREAM*)STDERR_FILENO, "WARNING: GC finalizers already enabled on this thread.\n");
             // Only print the backtrace once, to avoid spamming the logs
             static int backtrace_printed = 0;
             if (backtrace_printed == 0) {
@@ -421,7 +421,7 @@ JL_DLLEXPORT void jl_gc_enable_finalizers(jl_ptls_t ptls, int on)
         return;
     }
     ptls->finalizers_inhibited = new_val;
-    if (!new_val && old_val && !ptls->in_finalizer) {
+    if (!new_val && old_val && !ptls->in_finalizer && ptls->locks.len == 0) {
         ptls->in_finalizer = 1;
         run_finalizers(ptls);
         ptls->in_finalizer = 0;
@@ -3216,7 +3216,7 @@ JL_DLLEXPORT void jl_gc_collect(jl_gc_collection_t collection)
     // Only disable finalizers on current thread
     // Doing this on all threads is racy (it's impossible to check
     // or wait for finalizers on other threads without dead lock).
-    if (!ptls->finalizers_inhibited) {
+    if (!ptls->finalizers_inhibited && ptls->locks.len == 0) {
         int8_t was_in_finalizer = ptls->in_finalizer;
         ptls->in_finalizer = 1;
         run_finalizers(ptls);

src/julia.h

@@ -1733,7 +1733,6 @@ typedef struct _jl_handler_t {
     int8_t gc_state;
     size_t locks_len;
     sig_atomic_t defer_signal;
-    int finalizers_inhibited;
     jl_timing_block_t *timing_stack;
     size_t world_age;
 } jl_handler_t;

src/locks.h

@@ -84,11 +84,9 @@ static inline void jl_lock_frame_pop(void)
 
 static inline void jl_mutex_lock(jl_mutex_t *lock)
 {
-    jl_ptls_t ptls = jl_get_ptls_states();
     JL_SIGATOMIC_BEGIN();
     jl_mutex_wait(lock, 1);
     jl_lock_frame_push(lock);
-    jl_gc_enable_finalizers(ptls, 0);
 }
 
 static inline int jl_mutex_trylock_nogc(jl_mutex_t *lock)
@@ -111,10 +109,8 @@ static inline int jl_mutex_trylock(jl_mutex_t *lock)
 {
     int got = jl_mutex_trylock_nogc(lock);
     if (got) {
-        jl_ptls_t ptls = jl_get_ptls_states();
         JL_SIGATOMIC_BEGIN();
         jl_lock_frame_push(lock);
-        jl_gc_enable_finalizers(ptls, 0);
     }
     return got;
 }
@@ -136,7 +132,10 @@ static inline void jl_mutex_unlock(jl_mutex_t *lock)
     jl_mutex_unlock_nogc(lock);
     jl_lock_frame_pop();
     JL_SIGATOMIC_END();
-    jl_gc_enable_finalizers(ptls, 1); // runs finalizers (may GC)
+    if (ptls->locks.len == 0 && ptls->finalizers_inhibited == 0) {
+        ptls->finalizers_inhibited = 1;
+        jl_gc_enable_finalizers(ptls, 1); // call run_finalizers (may GC)
+    }
 }
 
 static inline void jl_mutex_init(jl_mutex_t *lock) JL_NOTSAFEPOINT

src/rtutils.c

@@ -218,7 +218,6 @@ JL_DLLEXPORT void jl_enter_handler(jl_handler_t *eh)
     eh->gc_state = ptls->gc_state;
     eh->locks_len = ptls->locks.len;
     eh->defer_signal = ptls->defer_signal;
-    eh->finalizers_inhibited = ptls->finalizers_inhibited;
     eh->world_age = ptls->world_age;
     current_task->eh = eh;
 #ifdef ENABLE_TIMINGS
@@ -249,14 +248,14 @@ JL_DLLEXPORT void jl_eh_restore_state(jl_handler_t *eh)
     current_task->eh = eh->prev;
     ptls->pgcstack = eh->gcstack;
     small_arraylist_t *locks = &ptls->locks;
-    if (locks->len > eh->locks_len) {
-        for (size_t i = locks->len;i > eh->locks_len;i--)
+    int unlocks = locks->len > eh->locks_len;
+    if (unlocks) {
+        for (size_t i = locks->len; i > eh->locks_len; i--)
             jl_mutex_unlock_nogc((jl_mutex_t*)locks->items[i - 1]);
         locks->len = eh->locks_len;
     }
     ptls->world_age = eh->world_age;
     ptls->defer_signal = eh->defer_signal;
-    ptls->finalizers_inhibited = eh->finalizers_inhibited;
     if (old_gc_state != eh->gc_state) {
         jl_atomic_store_release(&ptls->gc_state, eh->gc_state);
         if (old_gc_state) {
@@ -266,6 +265,11 @@ JL_DLLEXPORT void jl_eh_restore_state(jl_handler_t *eh)
     if (old_defer_signal && !eh->defer_signal) {
         jl_sigint_safepoint(ptls);
     }
+    if (unlocks && eh->locks_len == 0 && ptls->finalizers_inhibited == 0) {
+        // call run_finalizers
+        ptls->finalizers_inhibited = 1;
+        jl_gc_enable_finalizers(ptls, 1);
+    }
 }
 
 JL_DLLEXPORT void jl_pop_handler(int n)

test/misc.jl

@@ -100,9 +100,12 @@ let
         """
 end
 
+# Debugging tool: return the current state of the enable_finalizers counter.
+enable_finalizers_count() = ccall(:jl_gc_get_enable_finalizers, Int32, (Ptr{Cvoid},), C_NULL)
+
 # lock / unlock
 let l = ReentrantLock()
-    lock(l)
+    @test lock(l) === nothing
     @test islocked(l)
     success = Ref(false)
     @test trylock(l) do
@@ -115,17 +118,17 @@ let l = ReentrantLock()
     @test success[]
     t = @async begin
         @test trylock(l) do
-            @test false
+            error("unreachable")
         end === false
     end
+    @test enable_finalizers_count() == 1
     Base.wait(t)
-    unlock(l)
+    @test enable_finalizers_count() == 1
+    @test unlock(l) === nothing
+    @test enable_finalizers_count() == 0
     @test_throws ErrorException unlock(l)
 end
 
-# Debugging tool: return the current state of the enable_finalizers counter.
-enable_finalizers_count() = ccall(:jl_gc_get_enable_finalizers, Int32, (Ptr{Cvoid},), C_NULL)
-
 for l in (Threads.SpinLock(), ReentrantLock())
     @test enable_finalizers_count() == 0
     @test lock(enable_finalizers_count, l) == 1
@@ -137,14 +140,19 @@ for l in (Threads.SpinLock(), ReentrantLock())
         GC.enable_finalizers(true)
         @test enable_finalizers_count() == 1
     finally
-        @test enable_finalizers_count() == 0
-        GC.enable_finalizers(false)
         @test enable_finalizers_count() == 1
+        GC.enable_finalizers(false)
+        @test enable_finalizers_count() == 2
     end
+    @test enable_finalizers_count() == 2
+    GC.enable_finalizers(true)
     @test enable_finalizers_count() == 1
     GC.enable_finalizers(true)
     @test enable_finalizers_count() == 0
     @test_warn "WARNING: GC finalizers already enabled on this thread." GC.enable_finalizers(true)
+
+    @test lock(l) === nothing
+    @test try unlock(l) finally end === nothing
 end
 
 # task switching

test/threads_exec.jl

@@ -112,10 +112,10 @@ test_threaded_atomic_minmax(UInt16(27000),UInt16(37000))
 function threaded_add_locked(::Type{LockT}, x, n) where LockT
     critical = LockT()
     @threads for i = 1:n
-        lock(critical)
+        @test lock(critical) === nothing
         @test islocked(critical)
         x = x + 1
-        unlock(critical)
+        @test unlock(critical) === nothing
     end
     @test !islocked(critical)
     nentered = 0
@@ -124,7 +124,7 @@ function threaded_add_locked(::Type{LockT}, x, n) where LockT
         if trylock(critical)
             @test islocked(critical)
             nentered += 1
-            unlock(critical)
+            @test unlock(critical) === nothing
         else
             atomic_add!(nfailed, 1)
         end
@@ -142,21 +142,21 @@ end
 let critical = ReentrantLock()
     @test !islocked(critical)
     @test_throws ErrorException("unlock count must match lock count") unlock(critical)
-    lock(critical)
+    @test lock(critical) === nothing
     @test islocked(critical)
-    lock(critical)
-    t = trylock(critical); @test t
+    @test lock(critical) === nothing
+    @test trylock(critical) == true
     @test islocked(critical)
-    unlock(critical)
+    @test unlock(critical) === nothing
     @test islocked(critical)
-    unlock(critical)
+    @test unlock(critical) === nothing
     @test islocked(critical)
-    unlock(critical)
+    @test unlock(critical) === nothing
     @test !islocked(critical)
     @test_throws ErrorException("unlock count must match lock count") unlock(critical)
-    t = trylock(critical); @test t
+    @test trylock(critical) == true
     @test islocked(critical)
-    unlock(critical)
+    @test unlock(critical) === nothing
     @test !islocked(critical)
     @test_throws ErrorException("unlock count must match lock count") unlock(critical)
     @test !islocked(critical)
@@ -167,10 +167,10 @@ end
 function threaded_gc_locked(::Type{LockT}) where LockT
     critical = LockT()
     @threads for i = 1:20
-        lock(critical)
+        @test lock(critical) === nothing
         @test islocked(critical)
         GC.gc(false)
-        unlock(critical)
+        @test unlock(critical) === nothing
     end
     @test !islocked(critical)
 end