Skip to content

Commit

Permalink
Safely stop the heartbeat thread
Browse files Browse the repository at this point in the history
Previously we were not ensuring that the heartbeat thread had stopped before
exiting, and were closing the context in an unsafe way. This was causing hangs
on Mac and segfaults on Windows.
  • Loading branch information
JamesWrigley committed Feb 14, 2025
1 parent 11a3438 commit 68e3717
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 5 deletions.
8 changes: 5 additions & 3 deletions src/IJulia.jl
Original file line number Diff line number Diff line change
Expand Up @@ -175,11 +175,13 @@ function Base.close(kernel::Kernel)
popdisplay()

Check warning on line 175 in src/IJulia.jl

View check run for this annotation

Codecov / codecov/patch

src/IJulia.jl#L174-L175

Added lines #L174 - L175 were not covered by tests

# Close all sockets
close(kernel.publish[])
close(kernel.raw_input[])
close(kernel.requests[])
close(kernel.control[])
close(kernel.heartbeat_context[])
close(kernel.publish[])
close(kernel.raw_input[])

Check warning on line 181 in src/IJulia.jl

View check run for this annotation

Codecov / codecov/patch

src/IJulia.jl#L178-L181

Added lines #L178 - L181 were not covered by tests

# Wait for the heartbeat thread
stop_heartbeat(kernel)

Check warning on line 184 in src/IJulia.jl

View check run for this annotation

Codecov / codecov/patch

src/IJulia.jl#L184

Added line #L184 was not covered by tests

# The waitloop should now be ready to exit
kernel.inited = false
Expand Down
4 changes: 2 additions & 2 deletions src/handlers.jl
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,8 @@ request](https://jupyter-client.readthedocs.io/en/latest/messaging.html#kernel-s
sending the reply this will exit the process.
"""
function shutdown_request(socket, kernel, msg)

Check warning on line 216 in src/handlers.jl

View check run for this annotation

Codecov / codecov/patch

src/handlers.jl#L216

Added line #L216 was not covered by tests
# stop heartbeat thread by closing the context
close(kernel.heartbeat_context[])
# stop heartbeat thread
stop_heartbeat(kernel)

Check warning on line 218 in src/handlers.jl

View check run for this annotation

Codecov / codecov/patch

src/handlers.jl#L218

Added line #L218 was not covered by tests

# In protocol 5.4 the shutdown reply moved to the control socket
shutdown_socket = VersionNumber(msg) >= v"5.4" ? kernel.control[] : kernel.requests[]
Expand Down
16 changes: 16 additions & 0 deletions src/heartbeat.jl
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,23 @@ function heartbeat_thread(heartbeat::Ptr{Cvoid})
end

function start_heartbeat(heartbeat, kernel)
heartbeat.linger = 0

Check warning on line 26 in src/heartbeat.jl

View check run for this annotation

Codecov / codecov/patch

src/heartbeat.jl#L25-L26

Added lines #L25 - L26 were not covered by tests
heartbeat_c = @cfunction(heartbeat_thread, Cint, (Ptr{Cvoid},))
ccall(:uv_thread_create, Cint, (Ptr{Int}, Ptr{Cvoid}, Ptr{Cvoid}),
kernel.heartbeat_threadid, heartbeat_c, heartbeat)
end

function stop_heartbeat(kernel)

Check warning on line 32 in src/heartbeat.jl

View check run for this annotation

Codecov / codecov/patch

src/heartbeat.jl#L32

Added line #L32 was not covered by tests
# First we call zmq_ctx_shutdown() to ensure that the zmq_proxy() call
# returns. We don't call ZMQ.close(::Context) directly because that
# currently isn't threadsafe:
# https://github.com/JuliaInterop/ZMQ.jl/issues/256
ZMQ.lib.zmq_ctx_shutdown(kernel.heartbeat_context[])
@ccall uv_thread_join(kernel.heartbeat_threadid::Ptr{Int})::Cint

Check warning on line 38 in src/heartbeat.jl

View check run for this annotation

Codecov / codecov/patch

src/heartbeat.jl#L37-L38

Added lines #L37 - L38 were not covered by tests

# Now that the heartbeat thread has joined and its guaranteed to no longer
# be working on the heartbeat socket, we can safely close it and then the
# context.
close(kernel.heartbeat[])
close(kernel.heartbeat_context[])

Check warning on line 44 in src/heartbeat.jl

View check run for this annotation

Codecov / codecov/patch

src/heartbeat.jl#L43-L44

Added lines #L43 - L44 were not covered by tests
end

0 comments on commit 68e3717

Please sign in to comment.