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.

Sign up for GitHub

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

Jump to bottom

http: emit timeout duration overflow warning sync #18906

Closed
wants to merge 2 commits into from
Closed

http: emit timeout duration overflow warning sync #18906

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Emit the TimeoutOverflowWarning synchronously, even when
still connecting, to get a better stack trace.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

http

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Feb 21, 2018
@addaleax addaleax changed the title http: emit timeout duration overlow warning sync http: emit timeout duration overflow warning sync Feb 21, 2018
@addaleax addaleax force-pushed the http-timeout-warning branch from d1c7192 to d27fbed Compare February 21, 2018 16:55
@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/16417/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 22, 2018
@benjamingr
Copy link
Member

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

@addaleax
http: emit timeout duration overflow warning sync
8543d2b 
Emit the `TimeoutOverflowWarning` synchronously, even when
still connecting, to get a better stack trace.
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2018
@addaleax
Copy link
Member Author

addaleax commented Mar 2, 2018

@nodejs/http The test I’m adding here is failing in some CI machines with:

    events.js:116
          throw er; // Unhandled 'error' event
          ^
    
    Error: read ECONNRESET
        at TCP.onread (net.js:625:25)

Could I ask you to take a look at the test? I’m pretty certain it’s not related to the code change here, but I’d be curious to know whether there’s anything obviously wrong with it…

apapirovski
apapirovski reviewed Mar 2, 2018
View reviewed changes
test/parallel/test-http-timeout-client-warning.js Outdated
http.request(`http://localhost:${server.address().port}`)
.setTimeout(2 ** 40)
.end(() => {
server.close();
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this would be considered an issue with the test... there's a race condition between the server being closed and the request being processed. Most of these http tests would put the server.close(); in an event listener for end rather than a callback. Not sure if this is expected behaviour or a bug though... I can see a case for both.

Copy link
Member

Choose a reason for hiding this comment

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

See something like https://github.com/nodejs/node/blob/master/test/parallel/test-http-client-upload.js for an example.

@addaleax addaleax force-pushed the http-timeout-warning branch from d27fbed to cecd7f5 Compare March 2, 2018 13:15
@addaleax addaleax force-pushed the http-timeout-warning branch from cecd7f5 to 3044474 Compare March 2, 2018 13:21
@addaleax
Copy link
Member Author

addaleax commented Mar 2, 2018

@apapirovski Okay, I think I got it right. I am surprised though, I wouldn’t think it’s possible for the req.end() callback to fire before the connection is actually established?

CI: https://ci.nodejs.org/job/node-test-commit/16605/

@apapirovski
Copy link
Member

@addaleax Yeah, I honestly don't know... As far as I can tell, no one that knows the http module well is around and contributing anymore, and it's so convoluted and full of edge case handling that it's really, really hard to get a proper understanding of everything that's going on. I've considered doing some refactoring in the past but not sure where to start even...

@apapirovski
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/13483/ (Windows completely failed last time around)

@addaleax
Copy link
Member Author

addaleax commented Mar 4, 2018

Landed in 648d668

@addaleax addaleax closed this Mar 4, 2018
@addaleax addaleax deleted the http-timeout-warning branch March 4, 2018 21:55
addaleax added a commit that referenced this pull request Mar 4, 2018
@addaleax
http: emit timeout duration overflow warning sync
648d668 
Emit the `TimeoutOverflowWarning` synchronously, even when
still connecting, to get a better stack trace.

With thanks to Anatoli Papirovski for providing helpful
tips when writing the test.

PR-URL: #18906
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@addaleax @MayaLekova
http: emit timeout duration overflow warning sync
d8dc2a2 
Emit the `TimeoutOverflowWarning` synchronously, even when
still connecting, to get a better stack trace.

With thanks to Anatoli Papirovski for providing helpful
tips when writing the test.

PR-URL: nodejs#18906
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apapirovski apapirovski apapirovski left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@addaleax @benjamingr @apapirovski @jasnell @lpinca @cjihrig @tniessen @BridgeAR @nodejs-github-bot