Repository navigation
stream, net, http, http2: destroy while active race #30864
Description
Activity
- changed the title
[-]stream, net, http, http2: destroy while active[/-][+]stream, net, http, http2: destroy while active race[/+]on Dec 9, 2019 I would say this is expected.
destroy()signifies abnormal termination, and possibly it could result in an error.Given @addaleax comment and reading a bit about fd I found the following paragraph from close
It is probably unwise to close file descriptors while they may be in use by system calls in other threads in the same process. Since a file descriptor may be reused, there are some obscure race conditions that may cause unintended side effects.
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?Reacted by Anna HenningsenYeah, I feel like it would be very tricky to get this right for anything that uses the threadpool.
Not sure I quite understand the implications here.
Network sockets luckily don’t (at least on Unix).
So I guess
netwould be a good next candidate to look into further applying your fix on?Yeah, I feel like it would be very tricky to get this right for anything that uses the threadpool.
Not sure I quite understand the implications here.
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.Network sockets luckily don’t (at least on Unix).
So I guess
netwould be a good next candidate to look into further applying your fix on?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.Reacted by Robert NagyBasically 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 calling close() concurrently.
Ah, thank you! That makes things much more understandable for me.
- added a commit that references this issue
on Dec 17, 2019 - added a commit that references this issue
on Dec 17, 2019 - added a commit that references this issue
on Feb 6, 2020
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.