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

fix: replace socketTimeout with bodyTimeout #460

Merged
merged 24 commits into from
Oct 26, 2020
Merged

fix: replace socketTimeout with bodyTimeout #460

merged 24 commits into from
Oct 26, 2020

Conversation

ronag
Copy link
Member

@ronag ronag commented Oct 25, 2020 •
edited
Loading

  • Replaces socketTimeout with a bodyTimeout parameter on request.
  • Moves headersTimeout to parameter on request.
  • Removes socketTimeout.

Removes unecessary retry logic.

Fixes: #447

@ronag ronag added the semver-major Features or fixes that will be included in the next semver major release label Oct 25, 2020
@ronag ronag requested a review from mcollina October 25, 2020 13:29
mcollina
mcollina approved these changes Oct 25, 2020
View reviewed changes
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, so much better.

Could you create a 2.x branch for fixes on that line before landing?

@ronag ronag changed the base branch from master to 2.x October 25, 2020 22:03
@ronag ronag force-pushed the body-timeout2 branch 5 times, most recently from af5717e to d885529 Compare October 25, 2020 22:42
@mcollina
Copy link
Member

I think this should land on master and we keep v2.x for bugfixes on the current line.

@ronag ronag changed the base branch from 2.x to master October 26, 2020 04:10
@ronag
Copy link
Member Author

ronag commented Oct 26, 2020

@mcollina FYI some additional changes:

  • Implement headersTimeout in js.
  • Remove requestTimeout since it's the same as headersTimeout.
  • bodyTimeout and headersTimeout lives on request instead of globally on client.

@ronag ronag force-pushed the body-timeout2 branch 4 times, most recently from ba734e4 to d7d3af0 Compare October 26, 2020 06:35
@mcollina
Copy link
Member

CI keeps failing.

ronag added 10 commits October 26, 2020 08:43
@ronag
fix: replace socketTimeout with bodyTimeout
0fdf7d3 
Replaces socketTimeout with a bodyTimeout parameter. Removes socketTimeout.

Removes unecessary retry logic.

Fixes: #447
@ronag
fixup
ffe4ee1
@ronag
fixup
93a3877
@ronag
fixup
f0cb4f7
@ronag
fixup
e7c8cbe
@ronag
fixup
88cec9a
@ronag
fixup
dbaf679
@ronag
fixup
3bc730b
@ronag
fixup
5131b2e
@ronag
fixup
dbcb126
mcollina
mcollina requested changes Oct 26, 2020
View reviewed changes
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.

can you please keep an alias to the previous option or throw if that is passed in?

@ronag
Copy link
Member Author

ronag commented Oct 26, 2020

can you please keep an alias to the previous option or throw if that is passed in?

@mcollina fixed

@ronag ronag force-pushed the body-timeout2 branch 2 times, most recently from a0254f2 to f3832b1 Compare October 26, 2020 10:50
mcollina
mcollina approved these changes Oct 26, 2020
View reviewed changes
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 merged commit f1e8e14 into master Oct 26, 2020
@ronag ronag deleted the body-timeout2 branch October 26, 2020 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
semver-major Features or fixes that will be included in the next semver major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't timeout if socket is paused?
2 participants
@ronag @mcollina