RFC: uv_shutdown in julia close

[rsofaer]

This pull request resolves issue #2443.

I create a function is_open so that I can find out whether a stream has been marked closed in Julia from the C side, and call it in jl_close_uv to make sure shutdown isn't called twice.

If shutdown is called twice, my test will hang rather than fail. I couldn't figure out how to make that happen well.

I also didn't condense the switch statement in init.c, since I'm not sure whether those separations are still meaningful.

Thanks @vtjnash for helping me a lot with the Julia internals.

[JeffBezanson]

Thank you so much for working on this.

[JeffBezanson]

Do you have any thoughts about #1923?

We are moving away from underscores, and is_open should be isopen.

[rsofaer]

I don't really have an opinion on #1923, I think I would need to learn more about how the streams respond to errors etc to help with that. The main thing stream.open is doing here is telling you whether close(stream) has been called.

[vtjnash]

this is a potential race condition?

[rsofaer]

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

[vtjnash]

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

[vtjnash]

LGTM. @loladiro ?

[rsofaer]

I'm suspicious of that travis build failure. It could be uv_shutdown being
called twice on a stream. Hopefully I'll be able to look at it tonight.

On Mon, May 13, 2013 at 10:12 AM, Jameson Nash [email protected]:

LGTM. @loladiro https://github.com/loladiro ?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3084#issuecomment-17813904
.

[Keno]

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

[vtjnash]

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

[rsofaer]

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
.

[vtjnash]

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

[rsofaer]

I think this is ready to go now. The build is passing, I'm not closing closing streams, and the Julia function is cached. Any new comments?

[vtjnash]

none from me