GeezerVison

@ronag Thinking about this more, the EBADF indicates that we should probably try to avoid this race condition altogether and only call fs.close() until after a fs.read() or fs.write() operation has finished … I’ll update the PR along those lines

@ronag PTAL

CI: https://ci.nodejs.org/job/node-test-pull-request/27454/

Should we apply the same fixes to net.Socket?

Just a thought. What if for whatever reason the io doesn’t complete? Do we need a timeout? Or does libuv handle that?

@ronag In that case, this PR delays the close() call along with it. I don’t think that’s a bad thing, though.

I would like to ask for @mcollina's take on this before merging. See, #30864 (comment).

CI: https://ci.nodejs.org/job/node-test-pull-request/27521/

CI: https://ci.nodejs.org/job/node-test-pull-request/27522/

@ronag I guess we can do that, but at the same time I think we should fix this bug.

@ronag I guess we can do that, but at the same time I think we should fix this bug.

Given @mcollina's answer in the linked comment I'm not sure he would agree with this PR. Another valid (?) solution is also to just add on('error') handlers in the failing test.

@ronag Let’s wait for him to comment. In my opinion EBADF just because of using async iterators over a file stream is very clearly a bug.

CI: https://ci.nodejs.org/job/node-test-pull-request/27544/

I mostly agree. My personal concern is whether this fix should apply everywhere (net, http, http2, quic etc...) or whether fs is a somehow an edge case?

just because of using async iterators over a file stream is very clearly a bug.

This is a bit strange for me though. Shouldn't the iterator be released once leaving the for block? How can a released async iterator cause an exception at destroy()? @mcollina @benjamingr what is the expect semantics of async iterators here? https://github.com/nodejs/node/blob/master/test/parallel/test-readline-async-iterators-destroy.js I would expect the iterator to be released and release any error listeners.

The readable stream will of course still 'error' (which might or might not make sense) and an error listener should be registered regardless?

I'm a bit conflicted by this change.
On one hand It seems clear that we should protect the call to _destroy() to when there is no I/O operation happening. On the other hand, destroy() should be a "safe" operation and waiting for asynchronous completion seems a bit off.

I think we should consider making the callback of destroy(err, cb) documented and part of the official API.

What do you think?

On one hand It seems clear that we should protect the call to _destroy() to when there is no I/O operation happening. On the other hand, destroy() should be a "safe" operation and waiting for asynchronous completion seems a bit off.

Yes, that's a bit contradictory. I guess it is "safe" to wait for I/O if we're under the assumption that it will always complete (or fail) within reasonable time, otherwise we might end up with a stuck stream without means to abort it, e.g. a socket trying to write to a server which is bugged/crashed/corrupt? It might be a case by case basis. In fs I would guess it's very unlikely it would not complete within reasonable time (if you exclude FUSE). Also depends on whether e.g. libuv or the os has some sort of timeout or error handling for this.

I think we should consider making the callback of destroy(err, cb) documented and part of the official API.

I think it should be part of the official API. Not sure how that helps us here though? Also, before making it public we should probably ensure the cb is invoked asynchronously (which is not the case today).

Yes, that's a bit contradictory. I guess it is "safe" to wait for I/O if we're under the assumption that it will always complete (or fail) within reasonable time, otherwise we might end up with a stuck stream without means to abort it, e.g. a socket trying to write to a server which is bugged/crashed/corrupt? It might be a case by case basis. In fs I would guess it's very unlikely it would not complete within reasonable time (if you exclude FUSE). Also depends on whether e.g. libuv or the os has some sort of timeout or error handling for this.

I agree.

I think it should be part of the official API. Not sure how that helps us here though? Also, before making it public we should probably ensure the cb is invoked asynchronously (which is not the case today).

I think documenting that is enough.

I'm a bit conflicted by this change.
On one hand It seems clear that we should protect the call to _destroy() to when there is no I/O operation happening.

I’m not sure if that’s always the case, but here it’s definitely problematic. Getting EBADF is bad enough but I think it’s even possible that data is read or written from the wrong file here if the race condition timing works out really badly.

On the other hand, destroy() should be a "safe" operation and waiting for asynchronous completion seems a bit off.

I think we should consider making the callback of destroy(err, cb) documented and part of the official API.

I’d be okay with that, yes 👍

Landed in 8a5c7f6