response is only finished if socket is detached #31

ronag · 2019-06-24T22:26:24Z

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 · 2019-06-24T22:58:38Z

Sorry, fixed!

ronag · 2019-06-24T23:25:47Z

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 · 2019-06-24T23:56:00Z

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

ronag · 2019-06-24T23:59:33Z

(!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 · 2019-06-25T00:06:18Z

index.js

+  if (msg.stream) {
+    // Http2ServerRequest
+    // Http2ServerResponse
+    return msg.stream.closed


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

ronag · 2019-06-25T07:16:26Z

Updated test

ronag · 2019-06-25T11:50:39Z

Fails in node < 0.4. Is that a problem?

ronag · 2019-07-02T16:20:10Z

Is there anything more I can do here?

dougwilson · 2019-07-02T16:24:39Z

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 · 2019-07-02T16:50:34Z

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

dougwilson · 2019-07-02T16:53:09Z

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 · 2019-07-02T17:54:23Z

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 · 2019-07-02T18:02:27Z

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 · 2019-07-02T18:24:50Z

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

dougwilson · 2019-07-02T18:31:19Z

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 · 2019-07-02T18:39:28Z

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 · 2019-07-12T11:08:36Z

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 · 2019-07-12T11:09:50Z

This is also relevant nodejs/node#28621

ronag · 2019-07-14T16:22:07Z

nodejs/node#28681

ronag · 2019-08-17T06:30:17Z

index.js

+    return (
+      msg.finished &&
+      msg.outputSize === 0 &&
+      (!socket || socket.writableLength === 0)


This has landed in node 12 as writableFinished.

Typo: ~~writtableFinished~~ writableFinished.

wesleytodd · 2024-07-30T21:12:35Z

Hey @ronag! I am trying to do some house cleaning and just stumbled on this and #32. Can you help me come up to speed on this and decide on next steps?

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.

At first I saw both of these as bug fixes, but this makes me question it. We can certainly major version rev this if necessary (and maybe should just to drop old node versions), but I think the main thing we want to ensure is that this behavior aligns with the tag line "Execute a callback when a HTTP request closes, finishes, or errors".

I don't want to nit pick the original purpose of this package, only to resolve this so that it best serves the users of this (directly or via express), so if that is landing this I am for it.

IamLizu · 2024-08-20T15:21:31Z

A quick question, the problem mentioned in #30, is this a correct reproduction?

const express = require('express');
const onFinished = require('on-finished');

const app = express();

app.get('/', (req, res) => {
    res.write('Initial data...\n');

    // End the response
    res.end('Response ended.\n');

    // Simulate an error after the response is ended
    setTimeout(() => {
        res.emit('error', new Error('An error occurred after .end()'));
    }, 1000);

    onFinished(res, (err) => {
        if (err) {
            console.log('Error occurred after response ended:', err.message);
        } else {
            console.log('Response finished.');
        }
    });
});

app.listen(3000, () => {
    console.log('App running on port 3000');
});

cc: @ronag @wesleytodd

wesleytodd · 2024-11-14T16:05:50Z

This is all from quite a long time ago. I think it is a reproduction, but without digging in again I don't want to say with certainty. I would look to you to prove this out if you would like to help land this.

dougwilson added needs tests pr labels Jun 24, 2019

dougwilson suggested changes Jun 24, 2019

View reviewed changes

ronag force-pushed the master branch from ac0c466 to fcd6fb3 Compare June 24, 2019 22:46

ronag changed the title ~~Minor fixes~~ finished is only finished if socket is detached Jun 24, 2019

ronag changed the title ~~finished is only finished if socket is detached~~ response is only finished if socket is detached Jun 24, 2019

ronag mentioned this pull request Jun 24, 2019

response.finished ambiguity nodejs/node#28412

Closed

ronag force-pushed the master branch 5 times, most recently from c5a5131 to 7c8d7f3 Compare June 24, 2019 23:55

ronag force-pushed the master branch 2 times, most recently from a20c73d to e73e839 Compare June 25, 2019 00:05

ronag commented Jun 25, 2019

View reviewed changes

ronag force-pushed the master branch 2 times, most recently from ec12f71 to 200fb77 Compare June 25, 2019 07:15

ronag mentioned this pull request Jul 8, 2019

http2: make compat finished match http/1 nodejs/node#24347

Closed

4 tasks

ronag mentioned this pull request Jul 13, 2019

[x] http: added closed property nodejs/node#28621

Closed

4 tasks

ronag mentioned this pull request Jul 14, 2019

http: writableFinished nodejs/node#28681

Closed

4 tasks

ronag force-pushed the master branch from 200fb77 to b80a5d6 Compare July 14, 2019 21:58

msg.finished is only finished if socket has been detached

b9c2fa4

ronag force-pushed the master branch from b80a5d6 to b9c2fa4 Compare July 14, 2019 21:59

ronag commented Aug 17, 2019

View reviewed changes

wesleytodd mentioned this pull request Jul 30, 2024

Express 5.0 - last push! expressjs/discussions#233

Closed

46 tasks

UlisesGascon mentioned this pull request Sep 10, 2024

Backlog for Express 6.x expressjs/discussions#267

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

response is only finished if socket is detached #31

response is only finished if socket is detached #31

ronag commented Jun 24, 2019 •

edited

Loading

dougwilson left a comment

ronag commented Jun 24, 2019

ronag commented Jun 24, 2019 •

edited

Loading

ronag commented Jun 24, 2019

ronag commented Jun 24, 2019 •

edited

Loading

ronag Jun 25, 2019 •

edited

Loading

ronag commented Jun 25, 2019

ronag commented Jun 25, 2019

ronag commented Jul 2, 2019

dougwilson commented Jul 2, 2019

ronag commented Jul 2, 2019

dougwilson commented Jul 2, 2019

ronag commented Jul 2, 2019 •

edited

Loading

dougwilson commented Jul 2, 2019

ronag commented Jul 2, 2019

dougwilson commented Jul 2, 2019

dougwilson commented Jul 2, 2019

ronag commented Jul 12, 2019 •

edited

Loading

ronag commented Jul 12, 2019

ronag commented Jul 14, 2019

ronag Aug 17, 2019 •

edited

Loading

jaydenseric Jan 18, 2020

wesleytodd commented Jul 30, 2024

IamLizu commented Aug 20, 2024

wesleytodd commented Nov 14, 2024

response is only finished if socket is detached #31

Are you sure you want to change the base?

response is only finished if socket is detached #31

Conversation

ronag commented Jun 24, 2019 • edited Loading

dougwilson left a comment

Choose a reason for hiding this comment

ronag commented Jun 24, 2019

ronag commented Jun 24, 2019 • edited Loading

ronag commented Jun 24, 2019

ronag commented Jun 24, 2019 • edited Loading

ronag Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

ronag commented Jun 25, 2019

ronag commented Jun 25, 2019

ronag commented Jul 2, 2019

dougwilson commented Jul 2, 2019

ronag commented Jul 2, 2019

dougwilson commented Jul 2, 2019

ronag commented Jul 2, 2019 • edited Loading

dougwilson commented Jul 2, 2019

ronag commented Jul 2, 2019

dougwilson commented Jul 2, 2019

dougwilson commented Jul 2, 2019

ronag commented Jul 12, 2019 • edited Loading

ronag commented Jul 12, 2019

ronag commented Jul 14, 2019

ronag Aug 17, 2019 • edited Loading

Choose a reason for hiding this comment

jaydenseric Jan 18, 2020

Choose a reason for hiding this comment

wesleytodd commented Jul 30, 2024

IamLizu commented Aug 20, 2024

wesleytodd commented Nov 14, 2024

ronag commented Jun 24, 2019 •

edited

Loading

ronag commented Jun 24, 2019 •

edited

Loading

ronag commented Jun 24, 2019 •

edited

Loading

ronag Jun 25, 2019 •

edited

Loading

ronag commented Jul 2, 2019 •

edited

Loading

ronag commented Jul 12, 2019 •

edited

Loading

ronag Aug 17, 2019 •

edited

Loading