threads: avoid deadlock from recursive lock acquire

Finalizers can't safely acquire many essential locks (such as the
iolock, to cleanup libuv objects) if they are run inside another lock.
Therefore, inhibit all finalizers on the thread until all locks are
released (previously, this was only true for our internal locks).

NEWS.md

@@ -71,6 +71,8 @@ Command-line option changes
 Multi-threading changes
 -----------------------
 
+* Locks now automatically inhibit finalizers from running, to avoid deadlock ([#TBD]).
+* New function `Base.Threads.foreach(f, channel::Channel)` for multithreaded `Channel` consumption ([#34543]).
 
 Build system changes
 --------------------
@@ -84,7 +86,6 @@ New library functions
 ---------------------
 
 * New function `Base.kron!` and corresponding overloads for various matrix types for performing Kronecker product in-place ([#31069]).
-* New function `Base.Threads.foreach(f, channel::Channel)` for multithreaded `Channel` consumption ([#34543]).
 * New function `Base.readeach(io, T)` for iteratively performing `read(io, T)` ([#36150]).
 * `Iterators.map` is added. It provides another syntax `Iterators.map(f, iterators...)`
   for writing `(f(args...) for args in zip(iterators...))`, i.e. a lazy `map` ([#34352]).

base/gcutils.jl

@@ -106,6 +106,19 @@ Control whether garbage collection is enabled using a boolean argument (`true` f
 """
 enable(on::Bool) = ccall(:jl_gc_enable, Int32, (Int32,), on) != 0
 
+"""
+    GC.enable_finalizers(on::Bool)
+
+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)
+
 """
     GC.@preserve x1 x2 ... xn expr
 

base/lock.jl

@@ -6,9 +6,24 @@ const ThreadSynchronizer = GenericCondition{Threads.SpinLock}
 """
     ReentrantLock()
 
-Creates a re-entrant lock for synchronizing [`Task`](@ref)s.
-The same task can acquire the lock as many times as required.
-Each [`lock`](@ref) must be matched with an [`unlock`](@ref).
+Creates a re-entrant lock for synchronizing [`Task`](@ref)s. The same task can
+acquire the lock as many times as required. Each [`lock`](@ref) must be matched
+with an [`unlock`](@ref).
+
+Calling 'lock' will also inhibit running of finalizers on that thread until the
+corresponding 'unlock'. Use of the standard lock pattern illustrated below
+should naturally be supported, but beware of inverting the try/lock order or
+missing the try block entirely (e.g. attempting to return with the lock still
+held):
+
+```
+lock(l)
+try
+    <atomic work>
+finally
+    unlock(l)
+end
+```
 """
 mutable struct ReentrantLock <: AbstractLock
     locked_by::Union{Task, Nothing}
@@ -50,6 +65,7 @@ function trylock(rl::ReentrantLock)
     if rl.reentrancy_cnt == 0
         rl.locked_by = t
         rl.reentrancy_cnt = 1
+        GC.enable_finalizers(false)
         got = true
     else
         got = false
@@ -77,6 +93,7 @@ function lock(rl::ReentrantLock)
             if rl.reentrancy_cnt == 0
                 rl.locked_by = t
                 rl.reentrancy_cnt = 1
+                GC.enable_finalizers(false)
                 break
             end
             try
@@ -118,6 +135,7 @@ function unlock(rl::ReentrantLock)
                 rethrow()
             end
         end
+        GC.enable_finalizers(true)
         unlock(rl.cond_wait)
     end
     return
@@ -139,6 +157,7 @@ function unlockall(rl::ReentrantLock)
             rethrow()
         end
     end
+    GC.enable_finalizers(true)
     unlock(rl.cond_wait)
     return n
 end

base/locks-mt.jl

@@ -61,10 +61,12 @@ Base.assert_havelock(l::SpinLock) = islocked(l) ? nothing : Base.concurrency_vio
 function lock(l::SpinLock)
     while true
         if _get(l) == 0
+            GC.enable_finalizers(false)
             p = _xchg!(l, 1)
             if p == 0
                 return
             end
+            GC.enable_finalizers(true)
         end
         ccall(:jl_cpu_pause, Cvoid, ())
         # Temporary solution before we have gc transition support in codegen.
@@ -74,13 +76,19 @@ end
 
 function trylock(l::SpinLock)
     if _get(l) == 0
-        return _xchg!(l, 1) == 0
+        GC.enable_finalizers(false)
+        p = _xchg!(l, 1)
+        if p == 0
+            return true
+        end
+        GC.enable_finalizers(true)
     end
     return false
 end
 
 function unlock(l::SpinLock)
     _set!(l, 0)
+    GC.enable_finalizers(true)
     ccall(:jl_cpu_wake, Cvoid, ())
     return
 end

src/gc.c

@@ -280,7 +280,7 @@ static void run_finalizer(jl_ptls_t ptls, jl_value_t *o, jl_value_t *ff)
         jl_printf((JL_STREAM*)STDERR_FILENO, "error in running finalizer: ");
         jl_static_show((JL_STREAM*)STDERR_FILENO, jl_current_exception());
         jl_printf((JL_STREAM*)STDERR_FILENO, "\n");
-        jlbacktrace(); // writen to STDERR_FILENO
+        jlbacktrace(); // written to STDERR_FILENO
     }
 }
 
@@ -392,10 +392,29 @@ static void run_finalizers(jl_ptls_t ptls)
     arraylist_free(&copied_list);
 }
 
+JL_DLLEXPORT int jl_gc_get_enable_finalizers(jl_ptls_t ptls)
+{
+    if (ptls == NULL)
+        ptls = jl_get_ptls_states();
+    return ptls->finalizers_inhibited;
+}
+
 JL_DLLEXPORT void jl_gc_enable_finalizers(jl_ptls_t ptls, int on)
 {
+    if (ptls == NULL)
+        ptls = jl_get_ptls_states();
     int old_val = ptls->finalizers_inhibited;
     int new_val = old_val + (on ? -1 : 1);
+    if (new_val < 0) {
+        JL_TRY {
+            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");
+            jlbacktrace(); // written to STDERR_FILENO
+        }
+        return;
+    }
     ptls->finalizers_inhibited = new_val;
     if (!new_val && old_val && !ptls->in_finalizer) {
         ptls->in_finalizer = 1;
@@ -1581,7 +1600,7 @@ STATIC_INLINE uintptr_t gc_read_stack(void *_addr, uintptr_t offset,
 JL_NORETURN NOINLINE void gc_assert_datatype_fail(jl_ptls_t ptls, jl_datatype_t *vt,
                                                   jl_gc_mark_sp_t sp)
 {
-    jl_printf(JL_STDOUT, "GC error (probable corruption) :\n");
+    jl_safe_printf("GC error (probable corruption) :\n");
     gc_debug_print_status();
     jl_(vt);
     gc_debug_critical_error();

src/locks.h

@@ -134,9 +134,9 @@ static inline void jl_mutex_unlock(jl_mutex_t *lock)
 {
     jl_ptls_t ptls = jl_get_ptls_states();
     jl_mutex_unlock_nogc(lock);
-    jl_gc_enable_finalizers(ptls, 1);
     jl_lock_frame_pop();
     JL_SIGATOMIC_END();
+    jl_gc_enable_finalizers(ptls, 1); // runs finalizers (may GC)
 }
 
 static inline void jl_mutex_init(jl_mutex_t *lock) JL_NOTSAFEPOINT

test/misc.jl

@@ -123,6 +123,30 @@ let l = ReentrantLock()
     @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)
+
+let l = ReentrantLock()
+    @test enable_finalizers_count() == 0
+    @test lock(enable_finalizers_count, l) == 1
+    @test enable_finalizers_count() == 0
+    try
+        GC.enable_finalizers(false)
+        GC.enable_finalizers(false)
+        @test enable_finalizers_count() == 2
+        GC.enable_finalizers(true)
+        @test enable_finalizers_count() == 1
+    finally
+        @test enable_finalizers_count() == 0
+        GC.enable_finalizers(false)
+        @test enable_finalizers_count() == 1
+    end
+    @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)
+end
+
 # task switching
 
 @noinline function f6597(c)