-
Notifications
You must be signed in to change notification settings - Fork 177
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
//WriteTimeout: HTTPTimeout, | ||
Addr: bind, | ||
Handler: &lp, | ||
IdleTimeout: HTTPTimeout, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Makes sense
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:
make
runs successfully./test.sh
pass