Node: stream, net, http, http2: destroy while active race

[ronag]

Given the context of https://github.com/nodejs/node/pull/30837.

Where basically we have the problem of destroying a handle while it's being actively used during read/write causing "unexpected" errors.

1. 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?

2. Can we find a generic solution in streams as to avoid this "race"? e.g. in Writable we could defer calling _destroy until the active write has completed or failed. Readable would be a bit more tricky without changing or adding to the API.

I'm unsure how big of a problem this actually is.

[ronag]

ping @mcollina @addaleax @lpinca @jasnell

[mcollina]

I would say this is _expected_. destroy() signifies abnormal termination, and possibly it could result in an error.

[ronag]

@mcollina: Hm, I think that also makes sense. Given that, what is your take on https://github.com/nodejs/node/pull/30837?

[ronag]

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 https://github.com/nodejs/node/pull/30837 for fs should "probably" also apply in other places where file descriptor reuse is possible?

[addaleax]

@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]

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.

Network sockets luckily don’t (at least on Unix).

So I guess net would be a good next candidate to look into further applying your fix on?

[addaleax]

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 calling close() concurrently.

Network sockets luckily don’t (at least on Unix).

So I guess net would 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.

[ronag]

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 calling close() concurrently.

Ah, thank you! That makes things much more understandable for me.

[ronag]

@addaleax If fs is the only thing that uses the threadpool and can encounter this race I think we can close this issue?

[addaleax]

@ronag It’s the only thing I could tell you right now that it’s affected by this issue, yes