unix,win: deal with handle read stop from alloc cb#3743
unix,win: deal with handle read stop from alloc cb#3743bnoordhuis wants to merge 1 commit intolibuv:v1.xfrom
Conversation
The iron rule is that libuv always hands back memory that you gave it. That means a call to `uv_alloc_cb` must always be followed by a corresponding call to `uv_read_cb`, even when the former calls `uv_close()` or `uv_read_stop()`, otherwise the program leaks memory. Refs: libuv#3740
|
|
||
| assert(stream->alloc_cb != NULL); | ||
| buf = uv_buf_init(NULL, 0); | ||
| stream->alloc_cb((uv_handle_t*)stream, 64 * 1024, &buf); |
There was a problem hiding this comment.
Maybe cache the stream->read_cb value for as long as you hold on to the allocation, so that a user can safely read_stop(); read_start() at any time without having to write a read_cb that is capable of processing allocations from an earlier alloc_cb?
There was a problem hiding this comment.
actually... there's still some weird behavior in the pathological case where a user calls read_stop(); read_start() with different callbacks inside the alloc_cb, which is that you might not detect that read_stop was canceled, and you might end up passing an allocation from one alloc_cb into a totally different read_cb.
The only thing I can think of to detect that is to cache both the stream->alloc_cb and stream->read_cb before calling alloc_cb, and detect if either have changed or if read_stop was called.
It's a weird enough situation that you could probably outlaw it instead of solving it.
| If libuv still owns memory from an earlier :c:type:`uv_alloc_cb` call, it | ||
| invokes the :c:type:`uv_read_cb` call once with `nread == 0` to give back | ||
| the memory. No further :c:type:`uv_read_cb` callbacks will be made. |
There was a problem hiding this comment.
The way this is written does not make any promises about how long libuv might hold on to the allocation. Is that intentional?
It seems like libuv is very close to the guarantee of "read_stop means no more read_cb", with one exception where a user does something pretty weird (call read_stop in an alloc_cb but return an allocation anyway). It would be nice to offer that guarantee to users, even with the exception, because a user can keep a simpler mental model of how their code will behave.
There was a problem hiding this comment.
Yes, it's intentional. With IOCP it's possible that the read callback is a long time coming.
There was a problem hiding this comment.
This makes sense... am I reading the libuv code correctly, that this only happens in src/win/tty.c?
There was a problem hiding this comment.
Looks like it to me also. But seems like we can change that to use ReadConsoleInputExW with the NOWAIT flag now, and avoid the considerable associated complexity (e.g. e51442b)
There was a problem hiding this comment.
Even with NOWAIT I wouldn't want to tighten the guarantees. For one, that would preclude moving to io_uring; it wants you to commit the memory upfront.
(Technically you don't have to but at that point it's just a clunkier epoll.)
|
SGTM. Julia's implementation doesn't do this (our |
|
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
|
seems to still be faring badly on windows |
The iron rule is that libuv always hands back memory that you gave it.
That means a call to
uv_alloc_cbmust always be followed by acorresponding call to
uv_read_cb, even when the former callsuv_close()oruv_read_stop(), otherwise the program leaks memory.Refs: #3740
Untested on Windows
but there is a chance it will do the right thing out of the box(edit: okay, maybe not)uv_udp_recv_stop()is up next.