Skip to content
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.

Sign up for GitHub

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

Jump to bottom

more fixes to I/O and threading #31733

Merged
merged 1 commit into from
Apr 22, 2019
Merged

more fixes to I/O and threading #31733

merged 1 commit into from
Apr 22, 2019

Conversation

JeffBezanson
Copy link
Sponsor Member

  • Make more libuv-related code tolerant of callbacks running on other threads.
  • Add a bit of delay before blocking, to make it less likely that @threads will lead to callbacks running on other threads.
  • GC needs to wake event loop before waiting at its barrier
  • Handle exceptions inside @threads a bit better

Hopefully helps #31713 and #31702.

@JeffBezanson JeffBezanson added domain:multithreading Base.Threads and related functionality kind:bugfix This change fixes an existing bug backport 1.2 labels Apr 15, 2019
vtjnash
vtjnash reviewed Apr 15, 2019
View reviewed changes
src/gc.c
@@ -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();
Copy link
Sponsor Member

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?

Copy link
Contributor

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.

src/partr.c Outdated Show resolved Hide resolved
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)
Copy link
Sponsor Member

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

@ararslan ararslan added the domain:io Involving the I/O subsystem: libuv, read, write, etc. label Apr 15, 2019
@JeffBezanson
Copy link
Sponsor Member Author

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 @threads, so we basically need all of it now.

@JeffBezanson
Copy link
Sponsor Member Author

@EthanAnderes @dcjones please try this branch.

@EthanAnderes
Copy link

Yep, this fixes all my tests. Thanks Jeff!

@dcjones
Copy link
Contributor

dcjones commented Apr 16, 2019

My code runs without issues on this branch. Thanks for the fix!

@Jutho
Copy link
Contributor

Jutho commented Apr 16, 2019

Also seems to fix my problems with multithreading in the tests of Strided.jl. Thanks!

vtjnash
vtjnash reviewed Apr 18, 2019
View reviewed changes
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)
Copy link
Sponsor Member

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)
Copy link
Sponsor Member

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

@JeffBezanson
more fixes to I/O and threading
f1ed52c 
hopefully helps #31713 and #31702

make more stream code thread safe
@KristofferC KristofferC mentioned this pull request Apr 20, 2019
Closed
58 tasks
@JeffBezanson JeffBezanson merged commit 773140e into master Apr 22, 2019
@JeffBezanson JeffBezanson deleted the jb/thriofixes branch April 22, 2019 18:06
KristofferC pushed a commit that referenced this pull request May 9, 2019
@JeffBezanson @KristofferC
more fixes to I/O and threading (#31733)
6d01613 
hopefully helps #31713 and #31702

make more stream code thread safe

(cherry picked from commit 773140e)
@Jutho Jutho mentioned this pull request May 22, 2019
Closed
@Jutho
Copy link
Contributor

Jutho commented May 22, 2019

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:
Jutho/Strided.jl#4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vtjnash vtjnash vtjnash left review comments

@yuyichao yuyichao yuyichao left review comments

Assignees
No one assigned
Labels
domain:io Involving the I/O subsystem: libuv, read, write, etc. domain:multithreading Base.Threads and related functionality kind:bugfix This change fixes an existing bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@JeffBezanson @EthanAnderes @dcjones @Jutho @vtjnash @yuyichao @KristofferC @ararslan