-
Notifications
You must be signed in to change notification settings - Fork 113
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
Fix file-descriptor leak #954
Conversation
Clone client transport from http.DefaultTransport instead of creating a zeroed transport struct. With an empty timeout-struct all timeout-values are zeroed disabling some timeouts and TTLs. See https://golang.org/src/net/http/transpor.go#L194 and following.
54cfec2
to
99e82fc
Compare
@jasson99 please test this fix |
@IljaN nice catch, for long term I think we should avoid to rely on the default transport and always create it explicitly without relying on package variables. |
Merge after @jasson99 validates. |
"github.com/cs3org/reva/pkg/token" | ||
"github.com/pkg/errors" | ||
"go.opencensus.io/plugin/ochttp" | ||
) | ||
|
||
// GetHTTPClient returns an http client with open census tracing support. |
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.
is the TODO still necessary?
Yes agree! We just need to find or tune the right values for our use-case. For now I assumed that the go-lang selected defaults should suite us. Any proposals? Cloning from DefaultTransport will ensure that we will get defaults for new parameters which might get added in future versions of golang. |
@IljaN I tested with this fix and it works now. |
Clone client transport from http.DefaultTransport instead of creating a zeroed transport struct.
With an empty timeout-struct all timeout-values are zeroed disabling some timeouts and TTLs.
See https://golang.org/src/net/http/transport.go#L194 and following.
Closes owncloud/product#100