-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
more fixes to I/O and threading #31733
Conversation
@@ -189,6 +189,8 @@ NOINLINE uintptr_t gc_get_stack_ptr(void) | |||
#ifdef JULIA_ENABLE_THREADING | |||
static void jl_gc_wait_for_the_world(void) | |||
{ | |||
if (jl_n_threads > 1) | |||
jl_wake_libuv(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuyichao shouldn't libuv be in a gc-safe region?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can spend time there without hitting other Julia runtime then yes. In which case we also need to make sure the transition back is emitted for all the callbacks.
base/stream.jl
Outdated
@@ -555,7 +555,9 @@ function uv_readcb(handle::Ptr{Cvoid}, nread::Cssize_t, buf::Ptr{Cvoid}) | |||
if nread < 0 | |||
if nread == UV_ENOBUFS && nrequested == 0 | |||
# remind the client that stream.buffer is full | |||
notify(stream.readnotify) | |||
if !isempty(stream.readnotify) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should already be the first line of notify
, we shouldn't need to repeat this test twice in a row
835bf93
to
c8ac6b4
Compare
Ok, I've tried converting most of the stream Conditions to thread-safe variants. I was hoping to avoid a change that invasive for now, but it's impossible to predict which callbacks will happen while another thread is in the event loop, even if you don't do any I/O inside |
@EthanAnderes @dcjones please try this branch. |
Yep, this fixes all my tests. Thanks Jeff! |
My code runs without issues on this branch. Thanks for the fix! |
Also seems to fix my problems with multithreading in the tests of Strided.jl. Thanks! |
stdlib/Sockets/src/Sockets.jl
Outdated
@@ -495,7 +505,12 @@ function listen(callback, server::Union{TCPSocket, UDPSocket}) | |||
elseif err != UV_EAGAIN | |||
uv_error("accept", err) | |||
else | |||
stream_wait(server, server.connectnotify) | |||
lock(server.cond) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lock
seems like it probably should be around the whole loop (esp. accept
)
test/threads.jl
Outdated
Threads.@threads for j in 1:1000 | ||
end | ||
@show i | ||
readline(rd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to reduce the chance of deadlock (esp. on windows), the readline should go before the @show
c8ac6b4
to
f1ed52c
Compare
I have not yet had time to check myself, but a user of my Strided.jl package reports that the problem has returned in 1.3-dev: |
@threads
will lead to callbacks running on other threads.@threads
a bit betterHopefully helps #31713 and #31702.