UV_CLOSING too late?

[dvv]

Hi!

I believe setting UV_CLOSING at https://github.com/joyent/libuv/blob/master/src/unix/core.c#L114 is too late, and should be done right after entrance into uv_close(). This would allow us to use uv_is_closing() to determine more robustly whether a handle has already been attempted to close.

--Vladimir

[bnoordhuis]

I don't see why. You're not calling uv_close() and uv_is_closing() concurrently, are you?

[dvv]

scenario: uv_is_closing checks for UV_CLOSED|UV_CLOSING in handle flags. chances are in async flow, that i called uv_close() and then another piece of concurrent code called uv_is_closing() right in the moment when the first call haven't set UV_CLOSING yet. such code will execute uv_close(), the latter being called twice.

[bnoordhuis]

That's simply not safe. It's the responsibility of the libuv user to not call uv_async_send() (or any libuv function really) after uv_close() has been called on the async handle.

[dvv]

but how does the user know uv_close() has been called, really?

[bnoordhuis]

Synchronization between threads is left as an exercise to the reader. :-)

In all seriousness, you would use a uv_mutex_t and a flag to signal that the handle is closed.

[dvv]

i might be misunderstanding something, but if i start a timer to close a stream on timeout and simultaneously read from this stream (and must call uv_close() in case of IO error) -- in this scenario i don't see threads and mutexes. whatever event occurs the first, it must inhibit the second, and the order is completely indeterminate.

i can track the handle state in separate variable, but this seems quirky, as this is purely routine task.

[bnoordhuis]

i might be misunderstanding something, but if i start a timer to close a stream on timeout and simultaneously read from this stream (and must call uv_close() in case of IO error) -- in this scenario i don't see threads and mutexes. whatever event occurs the first, it must inhibit the second, and the order is completely indeterminate.

You are misunderstanding something. :-) In your example, everything runs serially. It doesn't matter when uv_close() sets the UV_CLOSING flag, the observable behavior is the same: the handle is in the open state before and in the closing state afterwards.

[saghul]

@dvv maybe this is the same thing you mentioned on #364?

[dvv]

@bnoordhuis from my POV handle is UV_CLOSING right upon entrance to uv_close()

@saghul not sure. this issue is about so far i don't see a robust way to make uv_close() idempotent, i.e. perform the task once. #364 is about one can (could?) make loop refcount negative with (seemingly?) valid sequence of operations on a timer.

[saghul]

@dvv I think I know where you are going. The way I understand it uv_close is not be idempotent. Particularly, I use the close_cb to free memory, so calling uv_close more than once would be a disaster. Now, if uv_close should just return if the handle is closing or closed is something @bnoordhuis probably has a good answer for :-)

I'm not sure if it would be such a good idea to make uv_close idempotent: on one hand I understand it could be handy, but OTOH it feels a bit like those macros which would silently ignore a call to free on a NULL pointer.

Those are my 2 cents :-) Thoughts?

[dvv]

i believe it's just a matter of the following guard code:

... uv_close(handle, cb) {
  if (handle->fd != -1 && !(handle->flags & (UV_CLOSING|UV_CLOSED))) {
    handle->flags |= UV_CLOSING;
    // proceed to real close...
    // ...
  }
}

This way we don't call cb more than once hence i see no way to double free

[bnoordhuis]

Right, that's a rather different issue. I don't want to make uv_close() idempotent, that's papering over application bugs. I am not opposed to adding an assert() though.

[saghul]

+1 for the assert, that would help detect the problem in the application easier.

[piscisaureus]

@dvv

libuv is not thread safe - you should not call uv_close from any other thread, unless you ensure that the main thread is blocked somewhere in user code. (so, before you ask, just suspending the thread isn't gonna cut it). Use a mutex or something.