diff --git a/base/gcutils.jl b/base/gcutils.jl index b73a73b657239..556172135c49e 100644 --- a/base/gcutils.jl +++ b/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) diff --git a/src/gc.c b/src/gc.c index ea62d8ba0a222..ac364b516134e 100644 --- a/src/gc.c +++ b/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); diff --git a/src/julia.h b/src/julia.h index a3addc636cbb6..e3c0a77502d10 100644 --- a/src/julia.h +++ b/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; diff --git a/src/locks.h b/src/locks.h index 0069e0922a0dc..b02fc8c5011e5 100644 --- a/src/locks.h +++ b/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 diff --git a/src/rtutils.c b/src/rtutils.c index 03fe6f461d564..9e755784efbec 100644 --- a/src/rtutils.c +++ b/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) diff --git a/test/misc.jl b/test/misc.jl index 33faa4b09eee8..9980d39dab2b3 100644 --- a/test/misc.jl +++ b/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 diff --git a/test/threads_exec.jl b/test/threads_exec.jl index c21ef45389f96..691fca2fb2afa 100644 --- a/test/threads_exec.jl +++ b/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