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

Always set request timeout on keep-alive connections #2447

Merged
merged 1 commit into from Nov 3, 2016

Conversation

Projects
None yet
2 participants
@mscdex
Copy link
Contributor

commented Nov 3, 2016

No description provided.

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

The changes in 001eae3 would cause the http request timeout to only get set if a new socket was being used. This PR makes sure it gets set even when we're already connected.

I apologize, I meant to slip this into the previous PR when I noticed it a little bit ago.

/cc @simov

@simov

This comment has been minimized.

Copy link
Member

commented Nov 3, 2016

I can publish another patch no problem, I had to ask you first. So is this good for merging/publishing?

@mscdex mscdex force-pushed the mscdex:fix-timeout-regression branch 2 times, most recently from 1c27101 to 910d710 Nov 3, 2016

Always set request timeout on keep-alive connections
001eae3 would erroneously only set http request timeout
if a new socket was being used for the request. This commit
ensures the http request timeout is always set, even on keep-alive
connections.

@mscdex mscdex force-pushed the mscdex:fix-timeout-regression branch from 910d710 to 82da8b8 Nov 3, 2016

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2016

It should be good to go now, I've added a test for it.

@simov simov merged commit 6301c90 into request:master Nov 3, 2016

3 checks passed

codecov/patch 100% of diff hit (target 92.45%)
Details
codecov/project 92.47% (+0.02%) compared to 7228f13
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@simov

This comment has been minimized.

Copy link
Member

commented Nov 3, 2016

Thank you! Published.

@mscdex mscdex deleted the mscdex:fix-timeout-regression branch Nov 15, 2016

pulkitsinghal added a commit to ShoppinPal/vend-nodejs-sdk that referenced this pull request Sep 15, 2017

Update dependencies `request` and `request-promise` ... all tests passed
## Motivation
- Some upstream projects that use `vend-nodejs-sdk` were facing strange
issues where long-running use (after 47 minutes or so) led so a situation
where nodejs would just hang after a POST request.
- There weren't any clear CPU or Memory consumption red flags.
- It seemed more like a bad promise chain or failing to invoke a callback
that often leads nodejs into a forever running state and there's nothing
really going on.

## Solution
Look at the [CHANGELOG](https://github.com/request/request/blob/master/CHANGELOG.md)
for `request` and see if any plausible reasons for trying a module update.

1. request/request#2431
2. request/request#2414
3. request/request#2447
4. request/request#2448

This wasn't an exact science but their descriptions seemed to merit
the dependency upgrade as "at least worth a try"

## Consequences
Dropped support for Node 0.10
request/request#2381
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.