From 5472d8bcf8f8dd8cdfbb4503539e5863208f9e15 Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Wed, 10 Jul 2024 23:19:08 +0200
Subject: [PATCH 01/14] make python GC always run on thread 1

---
 src/GC/GC.jl                 | 99 ++++++++++++++++++++++++++----------
 test/GC.jl                   | 57 ++++++++++++++++++++-
 test/finalize_test_script.jl |  9 ++++
 3 files changed, 137 insertions(+), 28 deletions(-)
 create mode 100644 test/finalize_test_script.jl

diff --git a/src/GC/GC.jl b/src/GC/GC.jl
index 0d1fa9a8..f28fcd8c 100644
--- a/src/GC/GC.jl
+++ b/src/GC/GC.jl
@@ -9,8 +9,19 @@ module GC
 
 using ..C: C
 
-const ENABLED = Ref(true)
-const QUEUE = C.PyPtr[]
+# `ENABLED`: whether or not python GC is enabled, or paused to process later
+const ENABLED = Threads.Atomic{Bool}(true)
+# this event allows us to `wait` in a task until GC is re-enabled
+const ENABLED_EVENT = Threads.Event()
+
+# this is the queue to process pointers for GC (`C.Py_DecRef`)
+const QUEUE = Channel{C.PyPtr}(Inf)
+
+# this is the task which performs GC from thread 1
+const GC_TASK = Ref{Task}()
+
+# This we use in testing to know when our GC is running
+const GC_FINISHED = Threads.Condition(ReentrantLock())
 
 """
     PythonCall.GC.disable()
@@ -24,6 +35,7 @@ Like most PythonCall functions, you must only call this from the main thread.
 """
 function disable()
     ENABLED[] = false
+    reset(ENABLED_EVENT)
     return
 end
 
@@ -39,47 +51,80 @@ Like most PythonCall functions, you must only call this from the main thread.
 """
 function enable()
     ENABLED[] = true
+    notify(ENABLED_EVENT)
+    return
+end
+
+function enqueue(ptr::C.PyPtr)
+    if ptr != C.PyNULL && C.CTX.is_initialized    
+        put!(QUEUE, ptr)
+    end
+    return
+end
+
+function enqueue_all(ptrs)
+    if C.CTX.is_initialized
+        for ptr in ptrs
+            put!(QUEUE, ptr)
+        end
+    end
+    return
+end
+
+# must only be called from thread 1 by the task in `GC_TASK[]`
+function unsafe_process_queue!()
     if !isempty(QUEUE)
         C.with_gil(false) do
-            for ptr in QUEUE
+            while !isempty(QUEUE) && ENABLED[]
+                # This should never block, since there should
+                # only be one consumer
+                # (we would like to not block while holding the GIL)
+                ptr = take!(QUEUE)
                 if ptr != C.PyNULL
                     C.Py_DecRef(ptr)
                 end
             end
         end
     end
-    empty!(QUEUE)
-    return
+    return nothing
 end
 
-function enqueue(ptr::C.PyPtr)
-    if ptr != C.PyNULL && C.CTX.is_initialized
-        if ENABLED[]
-            C.with_gil(false) do
-                C.Py_DecRef(ptr)
+function gc_loop()
+    while true
+        if ENABLED[] && !isempty(QUEUE)
+            unsafe_process_queue!()
+            # just for testing purposes
+            lock(GC_FINISHED)
+            try
+                notify(GC_FINISHED) 
+            finally
+                unlock(GC_FINISHED)
             end
-        else
-            push!(QUEUE, ptr)
         end
+        # wait until there is both something to process
+        # and GC is `enabled`
+        wait(QUEUE)
+        wait(ENABLED_EVENT)
     end
-    return
 end
 
-function enqueue_all(ptrs)
-    if C.CTX.is_initialized
-        if ENABLED[]
-            C.with_gil(false) do
-                for ptr in ptrs
-                    if ptr != C.PyNULL
-                        C.Py_DecRef(ptr)
-                    end
-                end
-            end
-        else
-            append!(QUEUE, ptrs)
-        end
+function launch_gc_task()
+    if isassigned(GC_TASK) && Base.istaskstarted(GC_TASK[]) && !Base.istaskdone(GC_TASK[])
+        throw(ConcurrencyViolationError("PythonCall GC task already running!"))
     end
-    return
+    task = Task(gc_loop)
+    task.sticky = VERSION >= v"1.7" # disallow task migration which was introduced in 1.7
+    # ensure the task runs from thread 1
+    ccall(:jl_set_task_tid, Cvoid, (Any, Cint), task, 0)
+    schedule(task)
+    Base.errormonitor(task)
+    GC_TASK[] = task
+    task
+end
+
+function __init__()
+    launch_gc_task()
+    nothing
 end
 
 end # module GC
diff --git a/test/GC.jl b/test/GC.jl
index 46409041..efd96dce 100644
--- a/test/GC.jl
+++ b/test/GC.jl
@@ -1 +1,56 @@
-# TODO
+@testset "201: GC segfaults" begin
+    # https://github.com/JuliaPy/PythonCall.jl/issues/201
+    # This should not segfault!
+    cmd = Base.julia_cmd()
+    path = joinpath(@__DIR__, "finalize_test_script.jl")
+    p = run(`$cmd -t2 --project $path`)
+    @test p.exitcode == 0
+end
+
+@testset "GC.enable() and GC.disable()" begin
+    PythonCall.GC.disable()
+    try
+        # Put some stuff in the GC queue
+        let
+            pyobjs = map(pylist, 1:100)
+            foreach(PythonCall.Core.py_finalizer, pyobjs)
+        end
+        # Since GC is disabled, we should have built up entries in the queue
+        # (there is no race since we push to the queue eagerly)
+        @test !isempty(PythonCall.GC.QUEUE)
+        # now, setup a task so we can know once GC has triggered.
+        # We start waiting *before* enabling GC, so we know we'll catch
+        # the notification that GC finishes
+        # We also don't enable GC until we get confirmation via `is_waiting[]`.
+        # At this point we have acquired the lock for `PythonCall.GC.GC_FINISHED`.
+        # We won't relinquish it until the next line where we `wait`,
+        # at which point the GC can trigger it. Therefore we should be certain
+        # that the ordering is correct and we can't miss the event.
+        is_waiting = Threads.Atomic{Bool}(false)
+        rdy = Threads.@spawn begin
+            lock(PythonCall.GC.GC_FINISHED)
+            try
+                is_waiting[] = true
+                wait(PythonCall.GC.GC_FINISHED)
+            finally
+                unlock(PythonCall.GC.GC_FINISHED)
+            end
+        end
+        # Wait until the task starts
+        ret = timedwait(5) do
+            istaskstarted(rdy) && is_waiting[]
+        end
+        @test ret == :ok
+        # Now, re-enable GC
+        PythonCall.GC.enable()
+        # Wait for GC to run
+        wait(rdy)
+        # There should be nothing left in the queue, since we fully process it.
+        @test isempty(PythonCall.GC.QUEUE)
+    finally
+        # Make sure we re-enable GC regardless of whether our tests pass
+        PythonCall.GC.enable()
+    end
+end
+
+@test_throws ConcurrencyViolationError("PythonCall GC task already running!") PythonCall.GC.launch_gc_task()
diff --git a/test/finalize_test_script.jl b/test/finalize_test_script.jl
new file mode 100644
index 00000000..b81e6647
--- /dev/null
+++ b/test/finalize_test_script.jl
@@ -0,0 +1,9 @@
+using PythonCall
+
+# This would consistently segfault pre-GC-thread-safety
+let
+    pyobjs = map(pylist, 1:100)
+    Threads.@threads for obj in pyobjs
+        PythonCall.Core.py_finalizer(obj)
+    end
+end

From 4cb1b4645fe4f907a6b098b59e7550c10bd5c608 Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Wed, 10 Jul 2024 23:24:10 +0200
Subject: [PATCH 02/14] unnecessary

---
 src/GC/GC.jl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/GC/GC.jl b/src/GC/GC.jl
index f28fcd8c..95add7ce 100644
--- a/src/GC/GC.jl
+++ b/src/GC/GC.jl
@@ -21,7 +21,7 @@ const QUEUE = Channel{C.PyPtr}(Inf)
 const GC_TASK = Ref{Task}()
 
 # This we use in testing to know when our GC is running
-const GC_FINISHED = Threads.Condition(ReentrantLock())
+const GC_FINISHED = Threads.Condition()
 
 """
     PythonCall.GC.disable()

From d34ef103844726a3694fb057146dc0a1a54f27da Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Wed, 10 Jul 2024 23:24:58 +0200
Subject: [PATCH 03/14] update docstrings

---
 src/GC/GC.jl | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/src/GC/GC.jl b/src/GC/GC.jl
index 95add7ce..1831fb65 100644
--- a/src/GC/GC.jl
+++ b/src/GC/GC.jl
@@ -26,12 +26,8 @@ const GC_FINISHED = Threads.Condition()
 """
     PythonCall.GC.disable()
 
-Disable the PythonCall garbage collector.
+Disable the PythonCall garbage collector. This should generally not be required.
 
-This means that whenever a Python object owned by Julia is finalized, it is not immediately
-freed but is instead added to a queue of objects to free later when `enable()` is called.
-
-Like most PythonCall functions, you must only call this from the main thread.
 """
 function disable()
     ENABLED[] = false
@@ -42,12 +38,8 @@ end
 """
     PythonCall.GC.enable()
 
-Re-enable the PythonCall garbage collector.
-
-This frees any Python objects which were finalized while the GC was disabled, and allows
-objects finalized in the future to be freed immediately.
+Re-enable the PythonCall garbage collector. This should generally not be required.
 
-Like most PythonCall functions, you must only call this from the main thread.
 """
 function enable()
     ENABLED[] = true

From 433d928c859e1ab621e34f903a9d2b31a32fef2a Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Wed, 10 Jul 2024 23:29:01 +0200
Subject: [PATCH 04/14] use `Base.@lock` for nicer syntax

---
 src/GC/GC.jl | 7 +------
 test/GC.jl   | 5 +----
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/src/GC/GC.jl b/src/GC/GC.jl
index 1831fb65..d728bf75 100644
--- a/src/GC/GC.jl
+++ b/src/GC/GC.jl
@@ -86,12 +86,7 @@ function gc_loop()
         if ENABLED[] && !isempty(QUEUE)
             unsafe_process_queue!()
             # just for testing purposes
-            lock(GC_FINISHED)
-            try
-                notify(GC_FINISHED) 
-            finally
-                unlock(GC_FINISHED)
-            end
+            Base.@lock GC_FINISHED notify(GC_FINISHED)
         end
         # wait until there is both something to process
         # and GC is `enabled`
diff --git a/test/GC.jl b/test/GC.jl
index efd96dce..9655925c 100644
--- a/test/GC.jl
+++ b/test/GC.jl
@@ -28,12 +28,9 @@ end
         # that the ordering is correct and we can't miss the event.
         is_waiting = Threads.Atomic{Bool}(false)
         rdy = Threads.@spawn begin
-            lock(PythonCall.GC.GC_FINISHED)
-            try
+            Base.@lock PythonCall.GC.GC_FINISHED begin
                 is_waiting[] = true
                 wait(PythonCall.GC.GC_FINISHED)
-            finally
-                unlock(PythonCall.GC.GC_FINISHED)
             end
         end
         # Wait until the task starts

From 364a6f874e1937280909a289d8a60afd84c3cfd0 Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Wed, 10 Jul 2024 23:34:22 +0200
Subject: [PATCH 05/14] use `errormonitor` conditionally

---
 src/GC/GC.jl | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/GC/GC.jl b/src/GC/GC.jl
index d728bf75..65e06998 100644
--- a/src/GC/GC.jl
+++ b/src/GC/GC.jl
@@ -104,7 +104,9 @@ function launch_gc_task()
     # ensure the task runs from thread 1
     ccall(:jl_set_task_tid, Cvoid, (Any, Cint), task, 0)
     schedule(task)
-    Base.errormonitor(task)
+    if isdefined(Base, :errormonitor)
+        Base.errormonitor(task)
+    end
     GC_TASK[] = task
     task
 end

From 1325e75cced59f20852f5f24d0304431201e1574 Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Wed, 10 Jul 2024 23:49:30 +0200
Subject: [PATCH 06/14] up

---
 src/GC/GC.jl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/GC/GC.jl b/src/GC/GC.jl
index 65e06998..adeae9d0 100644
--- a/src/GC/GC.jl
+++ b/src/GC/GC.jl
@@ -113,6 +113,7 @@ end
 
 function __init__()
     launch_gc_task()
+    enable() # start enabled
     nothing
 end
 

From e3a28fbc429537fbfa3871e59f087594ce744ddb Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Thu, 11 Jul 2024 10:01:41 +0200
Subject: [PATCH 07/14] add comment

---
 src/GC/GC.jl | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/GC/GC.jl b/src/GC/GC.jl
index adeae9d0..7025d9f7 100644
--- a/src/GC/GC.jl
+++ b/src/GC/GC.jl
@@ -12,6 +12,10 @@ using ..C: C
 # `ENABLED`: whether or not python GC is enabled, or paused to process later
 const ENABLED = Threads.Atomic{Bool}(true)
 # this event allows us to `wait` in a task until GC is re-enabled
+# we have both this and `ENABLED` since there is no `isready(::Event)`
+# for us to do a non-blocking check. Instead we must keep the event being triggered
+# in-sync with `ENABLED[]`.
+# We therefore modify both in `enable()` and `disable()` and nowhere else.
 const ENABLED_EVENT = Threads.Event()
 
 # this is the queue to process pointers for GC (`C.Py_DecRef`)

From 0271a9cc6f51d66f40dcf95a6d21d78ec98faf88 Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Sat, 13 Jul 2024 10:35:05 +0200
Subject: [PATCH 08/14] add some instrumentation

---
 src/GC/GC.jl | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/GC/GC.jl b/src/GC/GC.jl
index 7025d9f7..25105246 100644
--- a/src/GC/GC.jl
+++ b/src/GC/GC.jl
@@ -27,6 +27,9 @@ const GC_TASK = Ref{Task}()
 # This we use in testing to know when our GC is running
 const GC_FINISHED = Threads.Condition()
 
+# This is used for basic profiling
+const SECONDS_SPENT_IN_GC = Threads.Atomic{Float64}()
+
 """
     PythonCall.GC.disable()
 
@@ -53,16 +56,18 @@ end
 
 function enqueue(ptr::C.PyPtr)
     if ptr != C.PyNULL && C.CTX.is_initialized    
-        put!(QUEUE, ptr)
+        t = @elapsed put!(QUEUE, ptr)
+        Threads.atomic_add!(SECONDS_SPENT_IN_GC, t)
     end
     return
 end
 
 function enqueue_all(ptrs)
     if C.CTX.is_initialized
-        for ptr in ptrs
+        t = @elapsed for ptr in ptrs
             put!(QUEUE, ptr)
         end
+        Threads.atomic_add!(SECONDS_SPENT_IN_GC, t)
     end
     return
 end
@@ -88,7 +93,8 @@ end
 function gc_loop()
     while true
         if ENABLED[] && !isempty(QUEUE)
-            unsafe_process_queue!()
+            t = @elapsed unsafe_process_queue!()
+            Threads.atomic_add!(SECONDS_SPENT_IN_GC, t)
             # just for testing purposes
             Base.@lock GC_FINISHED notify(GC_FINISHED)
         end

From 49f736e8a5cf1b3eb50b4a17eedb8645a15e2f24 Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Sat, 13 Jul 2024 13:37:54 +0200
Subject: [PATCH 09/14] improve single-thread performance

---
 .gitignore   |   1 +
 gcbench.jl   |  43 ++++++++++++++++
 src/GC/GC.jl | 136 ++++++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 168 insertions(+), 12 deletions(-)
 create mode 100644 gcbench.jl

diff --git a/.gitignore b/.gitignore
index 399503ad..d5e8c5c1 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,4 +1,5 @@
 Manifest.toml
+Manifest-*.toml
 .ipynb_checkpoints
 __pycache__
 *.egg-info
diff --git a/gcbench.jl b/gcbench.jl
new file mode 100644
index 00000000..c3189fd9
--- /dev/null
+++ b/gcbench.jl
@@ -0,0 +1,43 @@
+using PythonCall, Test
+
+function wait_for_queue_to_be_empty()
+    ret = timedwait(5) do
+        isempty(PythonCall.GC.QUEUE)
+    end
+    ret === :ok || error("QUEUE still not empty")
+end
+
+# https://github.com/JuliaCI/GCBenchmarks/blob/fc288c696381ebfdef8f002168addd0ec1b08e34/benches/serial/append/append.jl
+macro gctime(ex)
+    quote
+        local prior = PythonCall.GC.SECONDS_SPENT_IN_GC[]
+        local ret = @timed $(esc(ex))
+        Base.time_print(stdout, ret.time * 1e9, ret.gcstats.allocd, ret.gcstats.total_time, Base.gc_alloc_count(ret.gcstats); msg="Runtime")
+        println(stdout)
+        local waiting = @elapsed wait_for_queue_to_be_empty()
+        local after = PythonCall.GC.SECONDS_SPENT_IN_GC[]
+        Base.time_print(stdout, (after - prior) * 1e9; msg="Python GC time")
+        println(stdout)
+        Base.time_print(stdout, waiting * 1e9; msg="Python GC time (waiting)")
+        println(stdout)
+        ret.value
+    end
+end
+
+function append_lots(iters=100 * 1024, size=1596)
+    v = pylist()
+    for i = 1:iters
+        v.append(pylist(rand(size)))
+    end
+    return v
+end
+
+GC.enable_logging(false)
+PythonCall.GC.enable_logging(false)
+@time "Total" begin
+    @gctime append_lots()
+    @time "Next full GC" begin
+        GC.gc(true)
+        wait_for_queue_to_be_empty()
+    end
+end
diff --git a/src/GC/GC.jl b/src/GC/GC.jl
index 25105246..7b201132 100644
--- a/src/GC/GC.jl
+++ b/src/GC/GC.jl
@@ -30,6 +30,18 @@ const GC_FINISHED = Threads.Condition()
 # This is used for basic profiling
 const SECONDS_SPENT_IN_GC = Threads.Atomic{Float64}()
 
+const LOGGING_ENABLED = Ref{Bool}(false)
+
+"""
+    PythonCall.GC.enable_logging(enable=true)
+
+Enables printed logging (similar to Julia's `GC.enable_logging`).
+"""
+function enable_logging(enable=true)
+    LOGGING_ENABLED[] = enable
+    return nothing
+end
+
 """
     PythonCall.GC.disable()
 
@@ -54,28 +66,121 @@ function enable()
     return
 end
 
-function enqueue(ptr::C.PyPtr)
-    if ptr != C.PyNULL && C.CTX.is_initialized    
-        t = @elapsed put!(QUEUE, ptr)
-        Threads.atomic_add!(SECONDS_SPENT_IN_GC, t)
+function enqueue_wrapper(f, g)
+    t = @elapsed begin
+        if C.CTX.is_initialized
+            # Eager path: if we are already on thread 1,
+            # we eagerly decrement
+            handled = false
+            if ENABLED[] && Threads.threadid() == 1
+                # temporarily disable thread migration to be sure
+                # we call `C.Py_DecRef` from thread 1
+                old_sticky = current_task().sticky
+                if !old_sticky
+                    current_task().sticky = true
+                end
+                if Threads.threadid() == 1
+                    f()
+                    # if ptr != C.PyNULL
+                    # C.Py_DecRef(ptr)
+                    # end
+                    handled = true
+                end
+                if !old_sticky
+                    current_task().sticky = old_sticky
+                end
+            end
+            if !handled
+                g()
+                # if ptr != C.PyNULL
+                # put!(QUEUE, ptr)
+                # end
+            end
+        end
     end
+    Threads.atomic_add!(SECONDS_SPENT_IN_GC, t)
     return
 end
 
-function enqueue_all(ptrs)
-    if C.CTX.is_initialized
-        t = @elapsed for ptr in ptrs
+function enqueue(ptr::C.PyPtr)
+    # if we are on thread 1:
+    f = () -> begin
+        if ptr != C.PyNULL
+            C.Py_DecRef(ptr)
+        end
+    end
+    # otherwise:
+    g = () -> begin
+        if ptr != C.PyNULL
             put!(QUEUE, ptr)
         end
-        Threads.atomic_add!(SECONDS_SPENT_IN_GC, t)
     end
-    return
+    enqueue_wrapper(f, g)
 end
 
+function enqueue_all(ptrs)
+    # if we are on thread 1:
+    f = () -> begin
+        for ptr in ptrs
+            if ptr != C.PyNULL
+                C.Py_DecRef(ptr)
+            end
+        end
+    end
+    # otherwise:
+    g = () -> begin
+        for ptr in ptrs
+            if ptr != C.PyNULL
+                put!(QUEUE, ptr)
+            end
+        end
+    end
+    enqueue_wrapper(f, g)
+end
+
+# function enqueue_all(ptrs)
+#     t = @elapsed begin
+#         if C.CTX.is_initialized
+#             # Eager path: if we are already on thread 1,
+#             # we eagerly decrement
+#             handled = false
+#             if ENABLED[] && Threads.threadid() == 1
+#                 # temporarily disable thread migration to be sure
+#                 # we call `C.Py_DecRef` from thread 1
+#                 old_sticky = current_task().sticky
+#                 if !old_sticky
+#                     current_task().sticky = true
+#                 end
+#                 if Threads.threadid() == 1
+#                     for ptr in ptrs
+#                         if ptr != C.PyNULL
+#                             C.Py_DecRef(ptr)
+#                         end
+#                     end
+#                     handled = true
+#                 end
+#                 if !old_sticky
+#                     current_task().sticky = old_sticky
+#                 end
+#             end
+#             if !handled
+#                 for ptr in ptrs
+#                     if ptr != C.PyNULL
+#                         put!(QUEUE, ptr)
+#                     end
+#                 end
+#             end
+#         end
+#     end
+#     Threads.atomic_add!(SECONDS_SPENT_IN_GC, t)
+#     return
+# end
+
 # must only be called from thread 1 by the task in `GC_TASK[]`
 function unsafe_process_queue!()
+    n = 0
     if !isempty(QUEUE)
-        C.with_gil(false) do
+        t = @elapsed C.with_gil(false) do
             while !isempty(QUEUE) && ENABLED[]
                 # This should never block, since there should
                 # only be one consumer
@@ -83,17 +188,24 @@ function unsafe_process_queue!()
                 ptr = take!(QUEUE)
                 if ptr != C.PyNULL
                     C.Py_DecRef(ptr)
+                    n += 1
                 end
             end
         end
+        if LOGGING_ENABLED[]
+            Base.time_print(stdout, t; msg="Python GC ($n items)")
+            println(stdout)
+        end
+    else
+        t = 0.0
     end
-    return nothing
+    return t
 end
 
 function gc_loop()
     while true
         if ENABLED[] && !isempty(QUEUE)
-            t = @elapsed unsafe_process_queue!()
+            t = unsafe_process_queue!()
             Threads.atomic_add!(SECONDS_SPENT_IN_GC, t)
             # just for testing purposes
             Base.@lock GC_FINISHED notify(GC_FINISHED)

From 2584d15b23f0882f4ad01a4a7bd948056e50d686 Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Sat, 13 Jul 2024 19:06:18 +0200
Subject: [PATCH 10/14] don't forget the GIL

---
 pytest/test_all.py | 15 +++++++++++++++
 src/GC/GC.jl       | 14 +++++++++-----
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/pytest/test_all.py b/pytest/test_all.py
index c6cff009..67e20942 100644
--- a/pytest/test_all.py
+++ b/pytest/test_all.py
@@ -75,3 +75,18 @@ def test_issue_433():
         """
     )
     assert out == 25
+
+def test_julia_gc():
+    from juliacall import Main as jl
+    # We make a bunch of python objects with no reference to them,
+    # then call GC to try to finalize them.
+    # We want to make sure we don't segfault.
+    jl.seval(
+        """
+        using PythonCall
+        let
+            pyobjs = map(pylist, 1:100)
+        end
+        GC.gc()
+        """
+    )
diff --git a/src/GC/GC.jl b/src/GC/GC.jl
index 7b201132..65afc2be 100644
--- a/src/GC/GC.jl
+++ b/src/GC/GC.jl
@@ -105,8 +105,10 @@ end
 function enqueue(ptr::C.PyPtr)
     # if we are on thread 1:
     f = () -> begin
-        if ptr != C.PyNULL
-            C.Py_DecRef(ptr)
+        C.with_gil(false) do
+            if ptr != C.PyNULL
+                C.Py_DecRef(ptr)
+            end
         end
     end
     # otherwise:
@@ -121,9 +123,11 @@ end
 function enqueue_all(ptrs)
     # if we are on thread 1:
     f = () -> begin
-        for ptr in ptrs
-            if ptr != C.PyNULL
-                C.Py_DecRef(ptr)
+        C.with_gil(false) do
+            for ptr in ptrs
+                if ptr != C.PyNULL
+                    C.Py_DecRef(ptr)
+                end
             end
         end
     end

From 854e8e9786de7493903e51352b229e7f8427682e Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Sun, 14 Jul 2024 12:49:11 +0200
Subject: [PATCH 11/14] test multithreaded in CI

---
 .github/workflows/tests.yml | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
index a3462b48..221fced8 100644
--- a/.github/workflows/tests.yml
+++ b/.github/workflows/tests.yml
@@ -14,12 +14,15 @@ jobs:
   julia:
     name: Test (${{ matrix.os }}, julia ${{ matrix.jlversion }})
     runs-on: ${{ matrix.os }}
+    env:
+      JULIA_NUM_THREADS: ${{ matrix.jl_nthreads }}
     strategy:
       fail-fast: true
       matrix:
         arch: [x64] # x86 unsupported by MicroMamba
         os: [ubuntu-latest, windows-latest, macos-latest]
-        jlversion: ['1','1.6']
+        jlversion: ['1', '1.6']
+        jl_nthreads: ['1', '2']
     steps:
       - uses: actions/checkout@v2
       - name: Set up Julia ${{ matrix.jlversion }}
@@ -52,11 +55,14 @@ jobs:
   python:
     name: Test (${{ matrix.os }}, python ${{ matrix.pyversion }})
     runs-on: ${{ matrix.os }}
+    env:
+      JULIA_NUM_THREADS: ${{ matrix.jl_nthreads }}
     strategy:
       fail-fast: true
       matrix:
         os: [ubuntu-latest, windows-latest, macos-latest]
         pyversion: ["3.x", "3.8"]
+        jl_nthreads: ['1', '2']
     steps:
       - uses: actions/checkout@v3
       - name: Set up Python ${{ matrix.pyversion }}

From dbdad787716fc2dd09442cae62c8c70f2ef89996 Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Sun, 14 Jul 2024 12:49:40 +0200
Subject: [PATCH 12/14] add comments, test multithreaded

---
 pytest/test_all.py           | 16 +++++++++++++++-
 test/finalize_test_script.jl |  2 +-
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/pytest/test_all.py b/pytest/test_all.py
index 67e20942..70a18808 100644
--- a/pytest/test_all.py
+++ b/pytest/test_all.py
@@ -81,12 +81,26 @@ def test_julia_gc():
     # We make a bunch of python objects with no reference to them,
     # then call GC to try to finalize them.
     # We want to make sure we don't segfault.
+    # Here we can (manually) verify that the background task is running successfully,
+    # by seeing the printout "Python GC (100 items): 0.000000 seconds."
+    # We also programmatically check things are working by verifying the queue is empty.
+    # Debugging note: if you get segfaults, then run the tests with
+    # `PYTHON_JULIACALL_HANDLE_SIGNALS=yes python3 -X faulthandler -m pytest -p no:faulthandler -s --nbval --cov=pysrc ./pytest/`
+    # in order to recover a bit more information from the segfault.
     jl.seval(
         """
-        using PythonCall
+        using PythonCall, Test
+        PythonCall.GC.enable_logging()
         let
             pyobjs = map(pylist, 1:100)
+            Threads.@threads for obj in pyobjs
+                finalize(obj)
+            end
         end
         GC.gc()
+        ret = timedwait(5) do
+            isempty(PythonCall.GC.QUEUE)
+        end
+        @test ret === :ok
         """
     )
diff --git a/test/finalize_test_script.jl b/test/finalize_test_script.jl
index b81e6647..ecacad9e 100644
--- a/test/finalize_test_script.jl
+++ b/test/finalize_test_script.jl
@@ -4,6 +4,6 @@ using PythonCall
 let
     pyobjs = map(pylist, 1:100)
     Threads.@threads for obj in pyobjs
-        PythonCall.Core.py_finalizer(obj)
+        finalize(obj)
     end
 end

From 6f42fc59468aef42db511340259266cbd908357f Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Sun, 14 Jul 2024 12:49:57 +0200
Subject: [PATCH 13/14] cleanup GC code (add comments, delete old code)

---
 src/GC/GC.jl | 46 ++--------------------------------------------
 1 file changed, 2 insertions(+), 44 deletions(-)

diff --git a/src/GC/GC.jl b/src/GC/GC.jl
index 65afc2be..718ba22c 100644
--- a/src/GC/GC.jl
+++ b/src/GC/GC.jl
@@ -66,6 +66,8 @@ function enable()
     return
 end
 
+# This is called within a finalizer so we must not task switch
+# (so no printing nor blocking on Julia-side locks)
 function enqueue_wrapper(f, g)
     t = @elapsed begin
         if C.CTX.is_initialized
@@ -81,9 +83,6 @@ function enqueue_wrapper(f, g)
                 end
                 if Threads.threadid() == 1
                     f()
-                    # if ptr != C.PyNULL
-                    # C.Py_DecRef(ptr)
-                    # end
                     handled = true
                 end
                 if !old_sticky
@@ -92,9 +91,6 @@ function enqueue_wrapper(f, g)
             end
             if !handled
                 g()
-                # if ptr != C.PyNULL
-                # put!(QUEUE, ptr)
-                # end
             end
         end
     end
@@ -142,44 +138,6 @@ function enqueue_all(ptrs)
     enqueue_wrapper(f, g)
 end
 
-# function enqueue_all(ptrs)
-#     t = @elapsed begin
-#         if C.CTX.is_initialized
-#             # Eager path: if we are already on thread 1,
-#             # we eagerly decrement
-#             handled = false
-#             if ENABLED[] && Threads.threadid() == 1
-#                 # temporarily disable thread migration to be sure
-#                 # we call `C.Py_DecRef` from thread 1
-#                 old_sticky = current_task().sticky
-#                 if !old_sticky
-#                     current_task().sticky = true
-#                 end
-#                 if Threads.threadid() == 1
-#                     for ptr in ptrs
-#                         if ptr != C.PyNULL
-#                             C.Py_DecRef(ptr)
-#                         end
-#                     end
-#                     handled = true
-#                 end
-#                 if !old_sticky
-#                     current_task().sticky = old_sticky
-#                 end
-#             end
-#             if !handled
-#                 for ptr in ptrs
-#                     if ptr != C.PyNULL
-#                         put!(QUEUE, ptr)
-#                     end
-#                 end
-#             end
-#         end
-#     end
-#     Threads.atomic_add!(SECONDS_SPENT_IN_GC, t)
-#     return
-# end
-
 # must only be called from thread 1 by the task in `GC_TASK[]`
 function unsafe_process_queue!()
     n = 0

From d84bb1b3ef336cf970baa6dbc22ad702e54f4db2 Mon Sep 17 00:00:00 2001
From: Eric Hanson <5846501+ericphanson@users.noreply.github.com>
Date: Sun, 14 Jul 2024 12:50:10 +0200
Subject: [PATCH 14/14] turn on signal handling for multithreaded julia

---
 pysrc/juliacall/__init__.py | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/pysrc/juliacall/__init__.py b/pysrc/juliacall/__init__.py
index 509dc415..9c1ff185 100644
--- a/pysrc/juliacall/__init__.py
+++ b/pysrc/juliacall/__init__.py
@@ -147,6 +147,17 @@ def args_from_config():
     CONFIG['opt_handle_signals'] = choice('handle_signals', ['yes', 'no'])[0]
     CONFIG['opt_startup_file'] = choice('startup_file', ['yes', 'no'])[0]
 
+    if CONFIG['opt_handle_signals'] is None:
+        cli_threads = CONFIG['opt_threads']
+        multi = isinstance(cli_threads, int) and cli_threads > 1
+        env_threads = os.environ.get('JULIA_NUM_THREADS', '0')
+        if env_threads == 'auto' or int(env_threads) > 1:
+            multi=True
+        if multi:
+            # We must handle signals if Julia has more than 1 thread.
+            # Should we issue a warning here that we are turning on signal-handling?
+            CONFIG['opt_handle_signals'] = "yes"
+
     # Stop if we already initialised
     if CONFIG['inited']:
         return
@@ -240,24 +251,6 @@ def jlstr(x):
 
     CONFIG['inited'] = True
 
-    if CONFIG['opt_handle_signals'] is None:
-        if Base.Threads.nthreads() > 1:
-            # a warning to help multithreaded users
-            # TODO: should we set PYTHON_JULIACALL_HANDLE_SIGNALS=yes whenever PYTHON_JULIACALL_THREADS != 1?
-            warnings.warn(
-                "Julia was started with multiple threads "
-                "but multithreading support is experimental in JuliaCall. "
-                "It is recommended to restart Python with the environment variable "
-                "PYTHON_JULIACALL_HANDLE_SIGNALS=yes "
-                "set, otherwise you may experience segfaults or other crashes. "
-                "Note however that this interferes with Python's own signal handling, "
-                "so for example Ctrl-C will not raise KeyboardInterrupt. "
-                "See https://juliapy.github.io/PythonCall.jl/stable/faq/#Is-PythonCall/JuliaCall-thread-safe? "
-                "for further information. "
-                "You can suppress this warning by setting "
-                "PYTHON_JULIACALL_HANDLE_SIGNALS=no."
-            )
-
     # Next, automatically load the juliacall extension if we are in IPython or Jupyter
     CONFIG['autoload_ipython_extension'] = choice('autoload_ipython_extension', ['yes', 'no'])[0]
     if CONFIG['autoload_ipython_extension'] in {'yes', None}: