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

RFC: uv_shutdown in julia close #3084

Merged
merged 8 commits into from
May 13, 2013
Merged

RFC: uv_shutdown in julia close #3084

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
59ce04d
Shut down all named pipes and tcp streams in jl_close_uv.
rsofaer May 9, 2013
fec04d9
Add a test for shutdown being called twice. Unfortunately it hangs r…
rsofaer May 11, 2013
aff5834
Stop using libuv's flags, check in jl_close_uv if close has already b…
rsofaer May 11, 2013
9c2051b
Change is_open to isopen
rsofaer May 11, 2013
10cc327
Test handle type before testing writable
rsofaer May 11, 2013
d1207a1
If a shutdownable stream is not writable, don't call uv_close on it.
rsofaer May 12, 2013
2609de0
Get jl_uv_close working again by checking uv_is_closing at the top an…
rsofaer May 13, 2013
4369e52
Use JULIA_CB to cache isopen
rsofaer May 13, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions base/exports.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1000,6 +1000,7 @@ export
bind,
connect,
close,
isopen,
countlines,
readcsv,
writecsv,
Expand Down
6 changes: 5 additions & 1 deletion base/stream.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,10 +558,14 @@ function link_pipe(read_end::Ptr{Void},readable_julia_only::Bool,write_end::Name
end
close_pipe_sync(handle::UVHandle) = ccall(:uv_pipe_close_sync,Void,(UVHandle,),handle)

function isopen(stream::AsyncStream)
stream.open
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a potential race condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do something like

if stream.state == OPEN
  stream.state = CLOSING
  ccall(...)
end

then mark it closed in the callback?

That means resolving #1923

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is my belief that resolving #1923 is the only safe thing to do overall. although it doesn't necessarily need to hold up the pull request since that issue exists independent of this fix


function close(stream::AsyncStream)
if stream.open
stream.open = false
ccall(:jl_close_uv,Void,(Ptr{Void},),stream.handle)
stream.open = false
end
end

Expand Down
20 changes: 3 additions & 17 deletions src/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,12 +246,7 @@ void sigint_handler(int sig, siginfo_t *info, void *context)

struct uv_shutdown_queue_item { uv_handle_t *h; struct uv_shutdown_queue_item *next; };
struct uv_shutdown_queue { struct uv_shutdown_queue_item *first; struct uv_shutdown_queue_item *last; };
static void jl_shutdown_uv_cb(uv_shutdown_t* req, int status)
{
//if (status == 0)
jl_close_uv((uv_handle_t*)req->handle);
free(req);
}

static void jl_uv_exitcleanup_add(uv_handle_t* handle, struct uv_shutdown_queue *queue)
{
struct uv_shutdown_queue_item *item = malloc(sizeof(struct uv_shutdown_queue_item));
Expand Down Expand Up @@ -300,17 +295,8 @@ DLLEXPORT void uv_atexit_hook()
//#endif
case UV_TCP:
case UV_NAMED_PIPE:
if (uv_is_writable((uv_stream_t*)handle)) { // uv_shutdown returns an error if not writable
uv_shutdown_t *req = malloc(sizeof(uv_shutdown_t));
int err = uv_shutdown(req, (uv_stream_t*)handle, jl_shutdown_uv_cb);
if (err != 0) {
printf("shutdown err: %s\n", uv_strerror(uv_last_error(jl_global_event_loop())));
jl_close_uv(handle);
}
}
else {
jl_close_uv(handle);
}
// These will be shut down in jl_close_uv.
jl_close_uv(handle);
break;
//Don't close these directly, but rather let the GC take care of it
case UV_POLL:
Expand Down
26 changes: 25 additions & 1 deletion src/jl_uv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,12 @@ void closeHandle(uv_handle_t* handle)
free(handle);
}

void shutdownCallback(uv_shutdown_t* req, int status)
{
uv_close((uv_handle_t*) req->handle,&closeHandle);
free(req);
}

void jl_return_spawn(uv_process_t *p, int exit_status, int term_signal)
{
JULIA_CB(return_spawn,p->data,2,CB_INT32,exit_status,CB_INT32,term_signal);
Expand Down Expand Up @@ -290,7 +296,25 @@ DLLEXPORT void jl_close_uv(uv_handle_t *handle)
return;
if (handle->type==UV_TTY)
uv_tty_set_mode((uv_tty_t*)handle,0);
uv_close(handle,&closeHandle);

if (uv_is_writable((uv_stream_t*)handle) && // uv_shutdown returns an error if not writable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to check the handle type before calling uv_is_writable (just reorder the if statement conditions)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the isopen test will need to be swapped with the uv_is_writable test because libuv on windows annoyingly responds to a shutdown request by marking the stream not writable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why swapping the isopen test with the uv_is_writable test would help. It looks like uv_shutdown is doing the same thing on both platforms with regard to writability, if it isn't writable it aborts. We're always going to be able to check both things since we check the handle type first, and if either test fails, going into uv_shutdown is going to cause trouble. Clarify for me?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On windows, the is_writable attribute is removed by the call to uv_shutdown, but we don't want to jump to calling uv_close via the else clause if the shutdown call is repeated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I get it, good catch.

On Sat, May 11, 2013 at 7:57 PM, Jameson Nash [email protected]:

In src/jl_uv.c:

@@ -290,7 +296,25 @@ DLLEXPORT void jl_close_uv(uv_handle_t handle)
return;
if (handle->type==UV_TTY)
uv_tty_set_mode((uv_tty_t)handle,0);

  • uv_close(handle,&closeHandle);
  • if (uv_is_writable((uv_stream_t*)handle) && // uv_shutdown returns an error if not writable

On windows, the is_writable attribute is removed by the call to
uv_shutdown, but we don't want to jump to calling uv_close via the else
clause if the shutdown call is repeated

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3084/files#r4184516
.

(handle->type == UV_NAMED_PIPE || handle->type == UV_TCP)) {
// Make sure that the stream has not already been marked closed in Julia.
jl_function_t* jl_is_open = (jl_function_t*) jl_get_global(jl_base_module, jl_symbol("isopen"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably should be done one and cached. See the way callbacks work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right. although it's slightly more annoying than just caching, since during sysimg.jl compile we have two jl_base_module's to look in. using the JULIA_CB macro is probably the way to go here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will I need to modify the JULIA_CB macro to not look for that _uv_hook
string? I don't really feel ready to poke at those macros.

On Mon, May 13, 2013 at 4:25 PM, Jameson Nash [email protected]:

In src/jl_uv.c:

@@ -290,7 +296,26 @@ DLLEXPORT void jl_close_uv(uv_handle_t handle)
return;
if (handle->type==UV_TTY)
uv_tty_set_mode((uv_tty_t)handle,0);

  • uv_close(handle,&closeHandle);
  • if (handle->type == UV_NAMED_PIPE || handle->type == UV_TCP) {
  •    // Make sure that the stream has not already been marked closed in Julia, which would cause a hang.

  •    // and has not been marked not writable by libuv (eg. by calling uv_shutdown on Windows), which would cause an abort.

  •    jl_function_t\* jl_is_open = (jl_function_t*) jl_get_global(jl_base_module, jl_symbol("isopen"));

right. although it's slightly more annoying than just caching, since
during sysimg.jl compile we have two jl_base_module's to look in. using the
JULIA_CB macro is probably the way to go here

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3084/files#r4201936
.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to suggest fixing it in the other way, by adding a julia method _uv_hook_isopen() which just dispatches to isopen()

if ( jl_apply(jl_is_open, (jl_value_t **) &(handle->data), 1) == jl_false){
return;
}

uv_shutdown_t *req = malloc(sizeof(uv_shutdown_t));
int err = uv_shutdown(req, (uv_stream_t*)handle, &shutdownCallback);
if (err != 0) {
printf("shutdown err: %s\n", uv_strerror(uv_last_error(jl_global_event_loop())));
uv_close(handle, &closeHandle);
}
}
else{
uv_close(handle,&closeHandle);
}
}

DLLEXPORT void jl_uv_associate_julia_struct(uv_handle_t *handle, jl_value_t *data)
Expand Down
17 changes: 17 additions & 0 deletions test/spawn.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,20 @@ if false
@test success(ignorestatus(`false` | `false`))
@test success(ignorestatus(`false` & `false`))
end

# Here we test that if we close a stream with pending writes, we don't lose the writes.
str = ""
for i=1:1000
str = "$str\n $(randstring(10))"
end
stdout, stdin, proc = readandwrite(`cat -`)
write(stdin, str)
close(stdin)
str2 = readall(stdout)
@test str2 == str

# This test hangs if the end of run walk across uv streams calls shutdown on a stream that is shutting down.
file = tempname()
stdout, stdin, proc = readandwrite(`cat -` > file)
write(stdin, str)
close(stdin)