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

Fix file-descriptor leak #954

Merged
merged 1 commit into from
Jul 9, 2020
Merged

Conversation

IljaN
Copy link
Member

@IljaN IljaN commented Jul 8, 2020

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

@IljaN IljaN requested a review from labkode as a code owner July 8, 2020 13:20
@IljaN IljaN requested review from butonic and refs July 8, 2020 13:20
@IljaN IljaN self-assigned this Jul 8, 2020
@IljaN IljaN added the bug Something isn't working label Jul 8, 2020
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.
@IljaN IljaN force-pushed the set_golang_default_timeouts branch from 54cfec2 to 99e82fc Compare July 8, 2020 13:33
@individual-it
Copy link
Contributor

@jasson99 please test this fix

@labkode
Copy link
Member

labkode commented Jul 8, 2020

@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.

@labkode
Copy link
Member

labkode commented Jul 8, 2020

Merge after @jasson99 validates.

@IljaN
Copy link
Member Author

IljaN commented Jul 8, 2020

nofail

Hatch 0.25, 150 Concurrent

"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.
Copy link
Member

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?

@IljaN
Copy link
Member Author

IljaN commented Jul 8, 2020

...for long term I think we should avoid to rely on the default transport and always create it explicitly without relying on package variables.

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.

@jasson99
Copy link
Contributor

jasson99 commented Jul 9, 2020

@IljaN I tested with this fix and it works now.

@labkode labkode merged commit ff6bdaa into cs3org:master Jul 9, 2020
@IljaN IljaN deleted the set_golang_default_timeouts branch July 12, 2020 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate Crash from benchmark testing
5 participants