http: emit ECONNRESET if no 'aborted' listener by ronag · Pull Request #28677 · nodejs/node

ronag · 2019-07-14T11:30:44Z

Emits an ECONNRESET of response object when there is no listener for aborted.

Refs: #28172

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mcollina · 2019-07-14T11:34:24Z

Would you mind adding a unit test?

ronag · 2019-07-14T11:35:08Z

@mcollina @benjamingr docs and tests added

mcollina

LGTM

ronag · 2019-07-14T11:36:59Z

@mcollina Thank you so much for the suggestion. This would make my life much easier.

nodejs-github-bot · 2019-07-14T11:37:09Z

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

mcollina · 2019-07-14T11:37:10Z

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

mcollina

LGTM

ronag · 2019-07-14T12:08:44Z

Hm, I have more tests to fix...

ronag · 2019-07-14T12:48:38Z

@mcollina May I deprecate/remove ‘aborted’ from the docs as well? Or is that a step too far?

mcollina · 2019-07-14T12:56:24Z

I would not deprecate/remove at this point.

lpinca · 2019-07-14T13:37:05Z

I think this is a pretty big breaking change as an 'error' listener should always be added on res now (if there is no 'aborted' listener) or the process will crash. It was sufficient to have an 'error' listener of req before.

ronag · 2019-07-14T13:44:49Z

@lpinca not having an aborted listener on the response is currently quite a serious bug... and something I believe unfortunately is quite common. Currently it will fail silently.

From an API standpoint you should always have a error listener on the response object.

But yes, this could be a sem major?

lpinca · 2019-07-14T14:33:21Z

not having an aborted listener on the response is currently quite a serious bug...

Why? care to elaborate?

lib/_http_client.js

ronag · 2019-07-14T15:05:49Z

not having an aborted listener on the response is currently quite a serious bug...

Why? care to elaborate?

Because you would never finish... consider the following quite common pattern:

await new Promise((resolve, reject) => res
  .on('error', reject)
  .pipe(dst)
  .on('error', reject)
  .on('finish', resolve)
)

If 'aborted' this will never finish...

mcollina

Moving it to “change requested” as we need a new error code for this.

lpinca · 2019-07-14T15:13:39Z

Hmm unconvinced, the example above is a very specific use case and I'm not very happy with enforcing either an 'aborted' or an 'error' listener but I won't get in the way.

ronag · 2019-07-14T15:26:57Z

Hmm unconvinced, the example above is a very specific use case and I'm not very happy with enforcing either an 'aborted' or an 'error' listener but I won't get in the way.

Fair enough. The only other argument I can give is that it pretends to be a stream but doesn't follow the stream spec (if end is not emitted then error should be emitted).

ronag · 2019-07-14T15:29:09Z

@mcollina: Tried a different message.

mcollina

LGTM

mcollina · 2019-08-26T09:05:15Z

@lpinca what do you think?

@nodejs/tsc this is likely a significant breaking change, you might want to review again.

nodejs-github-bot · 2019-08-26T09:05:49Z

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

mcollina · 2019-08-26T09:05:58Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1982/

lpinca · 2019-08-26T12:54:35Z

No strong opinion. The rationale seems sensible but I don't know if it makes sense due to the big breaking change.

ronag · 2019-09-18T22:52:45Z

@Trott: Is anything blocking this?

nodejs-github-bot · 2019-09-19T05:33:46Z

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

Trott · 2019-09-19T05:34:44Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2003/

Trott · 2019-09-23T01:19:00Z

With this change, CITGM has new failures for ws (7.1.2) and koa (2.8.1).

ws:

Details

2 failing
   1) WebSocket
        Connection establishing
          "after each" hook:
      Uncaught Error: aborted
       at connResetException (internal/errors.js:566:14)
       at Socket.socketCloseListener (_http_client.js:364:27)
       at TCP.<anonymous> (net.js:658:12)
   2) WebSocket
        Connection establishing
          "before each" hook for "connects when pathname is not null":
      TypeError: Cannot read property 'call' of undefined
       at processImmediate (internal/timers.js:439:21)

koa:

Details

3 failing
   1) app.respond
        when ctx.respond === false
          should function (ctx):
      Error: aborted
       at connResetException (internal/errors.js:566:14)
       at Socket.socketCloseListener (_http_client.js:364:27)
       at TCP.<anonymous> (net.js:658:12)
       [use `--full-trace` to display the full stack trace]
   2) app.respond
        when ctx.respond === false
          should ignore set header after header sent:
      Error: aborted
       at connResetException (internal/errors.js:566:14)
       at Socket.socketCloseListener (_http_client.js:364:27)
       at TCP.<anonymous> (net.js:658:12)
       [use `--full-trace` to display the full stack trace]
   3) app.respond
        when ctx.respond === false
          should ignore set status after header sent:
      Error: aborted
       at connResetException (internal/errors.js:566:14)
       at Socket.socketCloseListener (_http_client.js:364:27)
       at TCP.<anonymous> (net.js:658:12)
       [use `--full-trace` to display the full stack trace]
   Error: mock stack null
   Error: ENOENT: no such file or directory, open 'does not exist'
   Error: ENOENT: no such file or directory, open 'does not exist'
   Error: ENOENT: no such file or directory, open 'does not exist'
   Error: ENOENT: no such file or directory, open 'does not exist'
   Error: ENOENT: no such file or directory, open 'does not exist'
   Error: boom!

Trott

CITGM results would seem to indicate that we have some work to do in the ecosystem before landing this. If the approvers of this PR disagree and think it can/should land at this time, feel free to clear this review.

ronag · 2019-09-23T07:17:08Z

@Trott:

koa: Had broken test which I have already fixed on master but is pending a release.
ws: I've created a PR to maintain current functionality ([fix] listen to 'aborted' to mute emitting of 'aborted' error. websockets/ws#1632). Though I do realize that @lpinca might not be too happy with it. A nicer fix is probably possible.

lpinca · 2019-09-23T13:05:18Z

Yes, I don't like it but I can live with it in ws. I'm actually more worried about similar breakage this can have on the ecosystem as per #28677 (comment).

I've seen a lot of code where no 'error' listener and no 'aborted' listener is added on the client side http.IncomingMessage because it just worked.

mcollina

Considering the breakage in the ecosystem, I don't think we should land this.

ronag · 2019-09-24T11:56:10Z

because it just worked.

Or it just seems to work... I think a lot of the code that don't register error or aborted are already subtly broken... it's just such an unusual error case in those scenarios that it is not noticed. I don't have anything concrete to back that claim though.

ronag · 2019-12-15T10:45:59Z

Considering the breakage in the ecosystem, I don't think we should land this.

I guess adding an option/flag to enable this behavior is not an option either? I find this whole 'aborted' thing very unfortunate when considering that we want the response to act like a stream. pipeline and finished does negate some of this though. However, I do see the breaking risks.

A process.'warning' if aborting and neither error nor aborted is registered?

@mcollina: Is there anything more that can be done here or should we close this?

mcollina · 2019-12-15T19:45:47Z

I would close this for now, yes

Server requests aka. IncomingMessage emits 'aborted' instead of 'error' which causes confusion when the object is used as a regular stream, i.e. if functions working on streams are passed a server request object they might not work properly unless they take this into account. Refs: nodejs/web-server-frameworks#41 PR-URL: #33172 Fixes: #28172 Refs: #28677 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 14, 2019

ronag force-pushed the fix-error-instead-of-aborted branch 2 times, most recently from 8ce5c8d to 24f4a00 Compare July 14, 2019 11:34

ronag force-pushed the fix-error-instead-of-aborted branch from 24f4a00 to 0289de3 Compare July 14, 2019 11:34

ronag changed the title ~~http: emit ECONNRESET if not aborted listener~~ http: emit ECONNRESET if no aborted listener Jul 14, 2019

ronag changed the title ~~http: emit ECONNRESET if no aborted listener~~ http: emit ECONNRESET if no 'aborted' listener Jul 14, 2019

mcollina approved these changes Jul 14, 2019

View reviewed changes

ronag force-pushed the fix-error-instead-of-aborted branch from 0289de3 to 37554a3 Compare July 14, 2019 11:36

mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 14, 2019

mcollina approved these changes Jul 14, 2019

View reviewed changes

benjamingr approved these changes Jul 14, 2019

View reviewed changes

lpinca reviewed Jul 14, 2019

View reviewed changes

lib/_http_client.js Outdated Show resolved Hide resolved

mcollina requested changes Jul 14, 2019

View reviewed changes

ronag force-pushed the fix-error-instead-of-aborted branch from 37554a3 to af06d83 Compare July 14, 2019 15:21

ronag force-pushed the fix-error-instead-of-aborted branch from af06d83 to 147e7e2 Compare July 14, 2019 15:29

mcollina approved these changes Aug 26, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 19, 2019

Trott requested changes Sep 23, 2019

View reviewed changes

ronag mentioned this pull request Sep 23, 2019

[fix] listen to 'aborted' to mute emitting of 'aborted' error. websockets/ws#1632

Closed

ronag requested a review from Trott September 23, 2019 13:34

mcollina requested changes Sep 24, 2019

View reviewed changes

ronag closed this Dec 15, 2019

darksabrefr mentioned this pull request Apr 14, 2020

Server side aborted request with Transfer-Encoding: chunked never resolve/reject sindresorhus/got#1096

Closed

2 tasks

This was referenced Apr 22, 2020

Missing 'aborted' listener szmarczak/http-timer#20

Closed

http: IncomingMessage does not emit error on premature close nodejs/web-server-frameworks#41

Closed

ronag mentioned this pull request Apr 30, 2020

http: 'aborted' IncomingMessage should 'error' #33172

Closed

4 tasks

nflaig mentioned this pull request Jul 24, 2023

chore: remove redundant eventstream error listener ChainSafe/lodestar#5798

Merged

Uh oh!

Conversation

ronag commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mcollina commented Jul 14, 2019

Uh oh!

ronag commented Jul 14, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Jul 14, 2019

Uh oh!

nodejs-github-bot commented Jul 14, 2019

Uh oh!

mcollina commented Jul 14, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Jul 14, 2019

Uh oh!

ronag commented Jul 14, 2019

Uh oh!

mcollina commented Jul 14, 2019

Uh oh!

lpinca commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Jul 14, 2019

Uh oh!

Uh oh!

ronag commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

lpinca commented Jul 14, 2019

Uh oh!

ronag commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jul 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Aug 26, 2019

Uh oh!

nodejs-github-bot commented Aug 26, 2019

Uh oh!

mcollina commented Aug 26, 2019

Uh oh!

lpinca commented Aug 26, 2019

Uh oh!

ronag commented Sep 18, 2019

Uh oh!

nodejs-github-bot commented Sep 19, 2019

Uh oh!

Trott commented Sep 19, 2019

Uh oh!

Trott commented Sep 23, 2019

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lpinca commented Sep 23, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Jul 14, 2019 •

edited

Loading

lpinca commented Jul 14, 2019 •

edited

Loading

ronag commented Jul 14, 2019 •

edited

Loading

ronag commented Jul 14, 2019 •

edited

Loading

ronag commented Jul 14, 2019 •

edited

Loading

ronag commented Jul 14, 2019 •

edited

Loading

ronag commented Sep 23, 2019 •

edited

Loading

ronag commented Sep 24, 2019 •

edited

Loading

ronag commented Dec 15, 2019 •

edited

Loading