response is only finished if socket is detached

[ronag]

Refs: #30

[dougwilson]

Thanks!

If these are two different fixes, please split into two PRs. If not, then no problem. There is no description or tests so I'm not sure (a) if they are the just one fix or two separate fixes (b) what specifically they are fixing and (c) if they consistently fix the issue across the supported Node.js versions this module supports.

[ronag]

Sorry, fixed!

[ronag]

This PR does make the situation better. But it does not fully solve the issue.

If end() is called before socket is emitted it will still give a false positive. I don't know how to fix that at the moment...

[ronag]

Ok, I think I've found a way that catches all edge cases and also doesn't break http/2 compat.

[ronag]

(!socket && msg.finished && msg.outputSize === 0)

!socket is true if msg has not connected or has closed
msg.finished checks that end() has been called
msg.outputSize === 0 checks that there is no pending data

The logic here is a bit complicated.

We can't just rely on !socket since it will be a false positive if still not connected yet. However, if finished is true that means that some data must have been queued on the msg object which means that outputSize > 0 unless a socket has been assigned (even if it no longer is assigned).

[ronag]

Note this is needed in order to not break http2 compat which does not implement outputSize.

[ronag]

Updated test

[ronag]

Fails in node < 0.4. Is that a problem?

[ronag]

Is there anything more I can do here?

[dougwilson]

Hi @ronag . Sorry I have been out of a computer at the moment. This is a core piece of Express, and was created in support of the Express project. It looks like there are two core issues that block merging this currently: there is uncovered code in the PR (the coveralls check failure) and the failed CI on supported platforms (< 4, as you noted above).

I may be able to help you get those two things fixed up, though, if you're not sure how. If you're stuck on getting the checks on the PR resolved, please let me know and I can lend a hand.

[ronag]

The coveralls and node < 4 are a bit out of my expertise.

[dougwilson]

It's no problem at all, I can help out. So for the coveralls, it would just indicate that it is unreached code, as in there is no tests added to cover the added branches/lines.

I can help construct tests and add the < 4 support if you can help me understand the changes in this PR and/or how you've been manually testing the changes, which I can help replicate into automated tests.

[ronag]

you can help me understand the changes in this PR and/or how you've been manually testing the changes, which I can help replicate into automated tests.

So the problem this is trying to fix is the fact that msg.finished only tells whether end() has been called or not, and not whether 'error', 'aborted' or 'end' has been emitted (i.e. the response has finished).

There is currently no easy way to query this information from the response object in the success scenario ((socket && !socket.writable) handles 'error'). But here is what I was able to figure out for the success case.

(!socket && msg.finished && msg.outputSize === 0)

• !socket is true if msg has not connected or has closed
• msg.finished checks that end() has been called
• msg.outputSize === 0 checks that there is no pending data

The logic here is a bit complicated.

We can't just rely on !socket since it will be a false positive if still not connected yet. However, if finished is true that means that some data must have been queued on the msg object which means that outputSize > 0 unless a socket has been assigned (even if it no longer is assigned).

I've updated the test to check that it is not finished after end.

Furthermore, I had to add some extra code (hence the coveralls failure) to ensure we don't break node http/2 compat which does not have a outputSize but does have a property to check what we are looking for, i.e. stream.closed.

I suspect node < 4 does not have outputSize which is why it fails.

[dougwilson]

Gotcha. So this module (in the 1.x form) does consider the response finished after res.end is called, even if the data has not been flushed completely yet. So it sounds like this is a proposal for a 2.0 anyway, where the definition of a "finished" response is changed, is that accurate?

[ronag]

Another way to phrase it. This PR fixes so that ‘isFinished’ is never true before ‘onFinished’ has been called.

[dougwilson]

Well, more like this module's design was just to add a callback to the definition of what node.js http server considered finished, i.e. the res.finished property. And it sounds like you're saying this module is, indeed, following with what node.js defines res.finished as, but your idea of what the word "finished" means in the definition of a http response differs. I get it.

And I'm not arguing otherwise, only noting that this is not a bug fix, because the module is operating exactly as intended. It can certainly be made to change this definition away from the node.js finished property as a new major iteration of this module if that is what is desired here.

But in the same vein, it actually sounds like the bug is that the callback this module tried to add on top of the res.finished is being called too late, that res.finished turns true quite some time before the callback is emitted? That would be the bug fix, in my eyes, where as this PR is a definition change of "finished".

Remember: this module was built around adding a callback to the res.finished property as it's core purpose. It's in the name of the module, even :)

[dougwilson]

Anyway, regardless, I still want to help land this pull request, so maybe we should focus on the tasks at hand for that 👍

So I had originally saw your multiple commit iterations, and there is the http/2 issue. Were you testing the changes by hand? If so, can you help me replicate that setup? I can do the leg work to then translate into automated test cases in the test suite.

[ronag]

So I had originally saw your multiple commit iterations, and there is the http/2 issue. Were you testing the changes by hand? If so, can you help me replicate that setup? I can do the leg work to then translate into automated test cases in the test suite.

I was just reviewing the code. I don't have a setup for h2.

[ronag]

This is also relevant nodejs/node#28621

[ronag]

nodejs/node#28681

[ronag]

This has landed in node 12 as writableFinished.

[jaydenseric]

Typo: writtableFinished writableFinished.