Skip to content

Commit

Permalink
fix IO deadlock condition
Browse files Browse the repository at this point in the history
when calling uv_shutdown on a handle being written to by another process
we might never get the UV__POLLOUT notification
but we also don't need to delay the uv_close call
if we have nothing to write

in the future, we should introduce a new `uv_drain` callback,
instead of continuing to abuse uv_shutdown for this purpose

fix #22832
  • Loading branch information
vtjnash committed Jul 21, 2017
1 parent 526b83e commit 8949a95
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 9 deletions.
12 changes: 7 additions & 5 deletions src/jl_uv.c
Original file line number Diff line number Diff line change
Expand Up @@ -197,16 +197,18 @@ JL_DLLEXPORT void jl_close_uv(uv_handle_t *handle)
}

if (handle->type == UV_NAMED_PIPE || handle->type == UV_TCP) {
uv_stream_t *stream = (uv_stream_t*)handle;
#ifdef _OS_WINDOWS_
if (((uv_stream_t*)handle)->stream.conn.shutdown_req) {
if (stream->stream.conn.shutdown_req) {
#else
if (((uv_stream_t*)handle)->shutdown_req) {
if (stream->shutdown_req) {
#endif
// don't close the stream while attempting a graceful shutdown
return;
}
if (uv_is_writable((uv_stream_t*)handle)) {
if (uv_is_writable(stream) && stream->write_queue_size != 0) {
// attempt graceful shutdown of writable streams to give them a chance to flush first
// TODO: introduce a uv_drain cb API instead of abusing uv_shutdown in this way
uv_shutdown_t *req = (uv_shutdown_t*)malloc(sizeof(uv_shutdown_t));
req->data = 0;
/*
Expand All @@ -218,12 +220,12 @@ JL_DLLEXPORT void jl_close_uv(uv_handle_t *handle)
* b) In case the stream is already closed, in which case uv_close would
* cause an assertion failure.
*/
uv_shutdown(req, (uv_stream_t*)handle, &jl_uv_shutdownCallback);
uv_shutdown(req, stream, &jl_uv_shutdownCallback);
return;
}
}

if (!uv_is_closing((uv_handle_t*)handle)) {
if (!uv_is_closing(handle)) {
// avoid double-closing the stream
if (handle->type == UV_TTY)
uv_tty_set_mode((uv_tty_t*)handle, UV_TTY_MODE_NORMAL);
Expand Down
34 changes: 30 additions & 4 deletions test/spawn.jl
Original file line number Diff line number Diff line change
Expand Up @@ -298,15 +298,25 @@ let out = Pipe(), echo = `$exename --startup-file=no -e 'print(STDOUT, " 1\t", r
@test iswritable(out)
close(out.in)
@test !isopen(out.in)
Sys.iswindows() || @test !isopen(out.out) # it takes longer to propagate EOF through the Windows event system
@test_throws ArgumentError write(out, "now closed error")
@test isreadable(out)
@test !iswritable(out)
if !Sys.iswindows()
# on UNIX, we expect the pipe buffer is big enough that the write queue was immediately emptied
# and so we should already be notified of EPIPE on out.out by now
# and the other task should have already managed to consume all of the output
# it takes longer to propagate EOF through the Windows event system
# since it appears to be unwilling to buffer as much data
@test !isopen(out.out)
@test !isreadable(out)
end
@test_throws ArgumentError write(out, "now closed error")
if Sys.iswindows()
# WINNT kernel does not provide a fast mechanism for async propagation
# WINNT kernel appears to not provide a fast mechanism for async propagation
# of EOF for a blocking stream, so just wait for it to catch up.
# This shouldn't take much more than 32ms.
Base.wait_close(out)
# it's closed now, but the other task is expected to be behind this task
# in emptying the read buffer
@test isreadable(out)
end
@test !isopen(out)
end
Expand Down Expand Up @@ -469,3 +479,19 @@ let c = `ls -l "foo bar"`
@test length(c) == 3
@test eachindex(c) == 1:3
end

## Deadlock in spawning a cmd (#22832)
# FIXME?
#let stdout = Pipe(), stdin = Pipe()
# Base.link_pipe(stdout, julia_only_read=true)
# Base.link_pipe(stdin, julia_only_write=true)
# p = spawn(pipeline(catcmd, stdin=stdin, stdout=stdout, stderr=DevNull))
# @async begin # feed cat with 2 MB of data (zeros)
# write(stdin, zeros(UInt8, 1048576 * 2))
# close(stdin)
# end
# sleep(0.5) # give cat a chance to fill the write buffer for stdout
# close(stdout.in) # make sure we can still close the write end
# @test sizeof(readstring(stdout)) == 1048576 * 2 # make sure we get all the data
# @test success(p)
#end

0 comments on commit 8949a95

Please sign in to comment.