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

http: emit ECONNRESET if no 'aborted' listener #28677

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 14, 2019

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

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 14, 2019
@ronag ronag force-pushed the fix-error-instead-of-aborted branch 2 times, most recently from 8ce5c8d to 24f4a00 Compare July 14, 2019 11:34
@mcollina
Copy link
Member

Would you mind adding a unit test?

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

ronag commented Jul 14, 2019

@mcollina @benjamingr docs and tests added

@ronag ronag changed the title http: emit ECONNRESET if not aborted listener http: emit ECONNRESET if no aborted listener Jul 14, 2019
@ronag ronag changed the title http: emit ECONNRESET if no aborted listener http: emit ECONNRESET if no 'aborted' listener Jul 14, 2019
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ronag ronag force-pushed the fix-error-instead-of-aborted branch from 0289de3 to 37554a3 Compare July 14, 2019 11:36
@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 14, 2019
@ronag
Copy link
Member Author

ronag commented Jul 14, 2019

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

@nodejs-github-bot
Copy link
Collaborator

1 similar comment
@mcollina
Copy link
Member

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ronag
Copy link
Member Author

ronag commented Jul 14, 2019

Hm, I have more tests to fix...

@ronag
Copy link
Member Author

ronag commented Jul 14, 2019

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

@mcollina
Copy link
Member

I would not deprecate/remove at this point.

@lpinca
Copy link
Member

lpinca commented Jul 14, 2019

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

ronag commented Jul 14, 2019

@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
Copy link
Member

lpinca commented Jul 14, 2019

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

Why? care to elaborate?

lib/_http_client.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member Author

ronag commented Jul 14, 2019

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...

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lpinca
Copy link
Member

lpinca commented Jul 14, 2019

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 ronag force-pushed the fix-error-instead-of-aborted branch from 37554a3 to af06d83 Compare July 14, 2019 15:21
@ronag
Copy link
Member Author

ronag commented Jul 14, 2019

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 ronag force-pushed the fix-error-instead-of-aborted branch from af06d83 to 147e7e2 Compare July 14, 2019 15:29
@ronag
Copy link
Member Author

ronag commented Jul 14, 2019

@mcollina: Tried a different message.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member

@lpinca what do you think?

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

@nodejs-github-bot
Copy link
Collaborator

@mcollina
Copy link
Member

@lpinca
Copy link
Member

lpinca commented Aug 26, 2019

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

@ronag
Copy link
Member Author

ronag commented Sep 18, 2019

@Trott: Is anything blocking this?

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 19, 2019

@Trott 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
Copy link
Member

Trott commented Sep 23, 2019

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

ws:

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:

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!

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ronag commented Sep 23, 2019

@Trott:

@lpinca
Copy link
Member

lpinca commented Sep 23, 2019

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.

@ronag ronag requested a review from Trott September 23, 2019 13:34
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ronag
Copy link
Member Author

ronag commented Sep 24, 2019

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

ronag commented Dec 15, 2019

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

I would close this for now, yes

@ronag ronag closed this Dec 15, 2019
ronag added a commit that referenced this pull request May 10, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants