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

Add timeout for HTTP connections #2628

Merged
merged 3 commits into from
Oct 20, 2022

Conversation

leszko
Copy link
Contributor

@leszko leszko commented Oct 18, 2022

What does this pull request do? Explain your changes. (required)
Add timeout for the idea HTTP connections to prevent "too many files open" error.

Specific updates (required)

How did you test each of these updates (required)

Tested transcoding with combined OT and split O/T topologies.

Does this pull request close any open issues?
fix #2621

Checklist:

@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Merging #2628 (8ebeb32) into master (2298436) will increase coverage by 0.00591%.
The diff coverage is 0.00000%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2628         +/-   ##
===================================================
+ Coverage   56.29268%   56.29859%   +0.00591%     
===================================================
  Files             88          88                 
  Lines          19038       19036          -2     
===================================================
  Hits           10717       10717                 
+ Misses          7739        7737          -2     
  Partials         582         582                 
Impacted Files Coverage Δ
server/rpc.go 71.81009% <0.00000%> (+0.42365%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2298436...8ebeb32. Read the comment docs.

Impacted Files Coverage Δ
server/rpc.go 71.81009% <0.00000%> (+0.42365%) ⬆️

@leszko leszko marked this pull request as ready for review October 19, 2022 09:20
//WriteTimeout: HTTPTimeout,
Addr: bind,
Handler: &lp,
IdleTimeout: HTTPTimeout,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have the other timeouts too? Maybe even just the more general Timeout? I'm not sure there's any reason for a request to take longer than that

https://medium.com/@nate510/don-t-use-go-s-default-http-client-4804cb19f779

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general we should, but currently it would break the split O/T functionality, because /transcodeResults endpoint would also have read/write timeouts and our transcoders do not tolerate it now...

So, in general you're right, but I think to make it happen it's not a quick fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Makes sense

@leszko leszko merged commit 0bde82c into livepeer:master Oct 20, 2022
@leszko leszko deleted the rafal/fix-too-many-files-open branch October 20, 2022 06:45
@cyberj0g cyberj0g mentioned this pull request Oct 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

accept4: too many open files; retrying in 1s
4 participants