Repository navigation
stream, net, http, http2: destroy while active race #30864
Description
Given the context of #30837.
Where basically we have the problem of destroying a handle while it's being actively used during read/write causing "unexpected" errors.
-
I believe we might have similar problems in net, http, http2 and also quic. Where we can destroy the handle while it's in active use. Is this something we need to address?
-
Can we find a generic solution in streams as to avoid this "race"? e.g. in
Writablewe could defer calling_destroyuntil the active write has completed or failed.Readablewould be a bit more tricky without changing or adding to the API.
I'm unsure how big of a problem this actually is.
Activity
[-]stream, net, http, http2: destroy while active[/-][+]stream, net, http, http2: destroy while active race[/+]ronag commentedon Dec 9, 2019
ping @mcollina @addaleax @lpinca @jasnell
mcollina commentedon Dec 9, 2019
I would say this is expected.
destroy()signifies abnormal termination, and possibly it could result in an error.ronag commentedon Dec 9, 2019
@mcollina: Hm, I think that also makes sense. Given that, what is your take on #30837?
ronag commentedon Dec 11, 2019
Given @addaleax comment and reading a bit about fd I found the following paragraph from close
So basically, we should (must?) wait for any pending operations before closing (
_destroy:ing) a file descriptor. The fix we applied in #30837 for fs should "probably" also apply in other places where file descriptor reuse is possible?addaleax commentedon Dec 11, 2019
@ronag Yeah, I feel like it would be very tricky to get this right for anything that uses the threadpool. Network sockets luckily don’t (at least on Unix).
ronag commentedon Dec 11, 2019
Not sure I quite understand the implications here.
So I guess
netwould be a good next candidate to look into further applying your fix on?addaleax commentedon Dec 11, 2019
Basically what the
close(2)man page says – if it’s possible that the fd is being used for a syscall in one thread of the threadpool, we should not be callingclose()concurrently.I’m not sure that they need it, because they don’t use the threadpool, unlike fs operations. Everything is single-threaded here, and once the handle is closed by
uv_close(), we won’t access it anymore.ronag commentedon Dec 11, 2019
Ah, thank you! That makes things much more understandable for me.
ronag commentedon Dec 11, 2019
@addaleax If
fsis the only thing that uses the threadpool and can encounter this race I think we can close this issue?addaleax commentedon Dec 11, 2019
@ronag It’s the only thing I could tell you right now that it’s affected by this issue, yes
doc: add note about fs.close() about undefined behavior
doc: add note about fs.close() about undefined behavior
doc: add note about fs.close() about undefined behavior