isFinished incorrect?

[ronag]

Looking at the implementation in NodeJS.

finished only means that end() has been called on the response object, not necessarily that it actually has "finished", i.e. all data is not guaranteed to have been sent nor is it guaranteed that no further errors will occur...

I think the definition of finished in Node is a little bit confusing. None the less, the semantics are well defined and I don't think the match the assumptions of this library...

[dougwilson]

Thanks. So what would need to be changed in this lib? Do you have a test case that can be added to the test suite to demonstrate the issue?

[ronag]

I think the following needs to be changed:

https://github.com/jshttp/on-finished/blob/master/index.js#L70

To

return Boolean((!socket && msg.finished) || (socket && !socket.writable))

i.e. !socket, since detachSocket will be called on response completion removing the socket reference from the response object.

This is a bit of a theoretical edge case so finding a test case might be a bit difficult.

[dougwilson]

I don't think the match the assumptions of this library...

What do you think the assumptions are of this library? Nothing in the README makes any guarantee that matches what it sounds like you are saying you think this module does. For example, that it returns true iff all bytes have been flushed to the network.

[dougwilson]

This is a bit of a theoretical edge case so finding a test case might be a bit difficult.

With no test case, then how do you know the change does anything different from what the current implementation does? How do we not regress back to "broken" behavior? A test case would need to accompany any change. I can help come up with a test case that works in the test suite even if the only test case you can make may not be fully Node.js.

[ronag]

What do you think the assumptions are of this library?

The listener will be invoked only once when the response finished. If the response finished to an error, the first argument will contain the error

The finished property on the NodeJS response does not mean that the response has "finished" just that the user has "finished" with the response (i.e. called end()). Furthermore, it does not guarantee that no further errors will be emitted. Both of which, although a bit ambiguous, is something I suspect most users of this library would assume.

With no test case, then how do you know the change does anything different from what the current implementation does?

If the existing tests pass then I would expect no regression in any currently well-defined behavior.

This is no biggie for me. I've used this library as a reference when discussing other node related issues. I'm hoping that either the current definition of finished change or another one is added that better corresponds with what most people expect... However, until then we should probably fix this?

[dougwilson]

So the argument here is that the behavior is not very well defined, so even if tests all pass with the changes, how do we know this won't create some subtle behavior change in dependent libraries and apps? Ideally, even without a test case (thought the change will not be accepted without one) a description on what, specifically, is being changed (i.e. fixed) here from the perspective of consumers of this library. So if this fixes a bug, consumers of this library would be exposed to said bug, ideally which the change log would note of what it is.

[ronag]

Yes, I've further raised issues over at nodejs.

https://github.com/nodejs/node/pull/28411 https://github.com/nodejs/node/issues/28412

I'm not actually sure what the best course of action here is. This is a potentially very subtle bug. Fixing it might break existing code. Not sure where this lands on the semantic versioning side of things.

So the argument here is that the behavior is not very well defined,

Something like that. I think the way response.finished is assumed to behave and how it actually does behave is not consistent.

[ronag]

This is only a problem if an error occurs after calling .end() on the response object. At the moment it is unclear to me whether or when this could actually occur in practice. In theory, I believe it is certainly a possibility that can cause very subtle bugs.