Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

isFinished incorrect? #30

Open
ronag opened this issue Jun 24, 2019 · 8 comments
Open

isFinished incorrect? #30

ronag opened this issue Jun 24, 2019 · 8 comments
Labels

Comments

@ronag
Copy link

ronag commented Jun 24, 2019

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
Copy link
Contributor

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
Copy link
Author

ronag commented Jun 24, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

ronag commented Jun 24, 2019

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
Copy link
Contributor

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
Copy link
Author

ronag commented Jun 24, 2019

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

nodejs/node#28411
nodejs/node#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
Copy link
Author

ronag commented Jun 24, 2019

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.

Trott pushed a commit to Trott/io.js that referenced this issue Jun 27, 2019
PR-URL: nodejs#28411
Refs: jshttp/on-finished#30
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
targos pushed a commit to nodejs/node that referenced this issue Jul 2, 2019
PR-URL: #28411
Refs: jshttp/on-finished#30
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@ronag ronag changed the title msg.finished incorrect? isFinished incorrect? Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants