From 1ca4a57325133877404d27ac1ca10917d4ab5bf5 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 20 Oct 2020 16:26:15 +0200 Subject: [PATCH 1/5] reuse http client on ocdav --- .../http/services/datagateway/datagateway.go | 11 +++++----- internal/http/services/owncloud/ocdav/copy.go | 22 +++++-------------- internal/http/services/owncloud/ocdav/get.go | 6 +---- .../http/services/owncloud/ocdav/ocdav.go | 18 ++++++++++----- internal/http/services/owncloud/ocdav/put.go | 8 ++----- internal/http/services/owncloud/ocdav/tus.go | 6 +---- 6 files changed, 27 insertions(+), 44 deletions(-) diff --git a/internal/http/services/datagateway/datagateway.go b/internal/http/services/datagateway/datagateway.go index 7b7549f84b..04b58499cb 100644 --- a/internal/http/services/datagateway/datagateway.go +++ b/internal/http/services/datagateway/datagateway.go @@ -20,12 +20,6 @@ package datagateway import ( "context" - "io" - "net/http" - "net/url" - "path" - "time" - "github.com/cs3org/reva/pkg/appctx" "github.com/cs3org/reva/pkg/errtypes" "github.com/cs3org/reva/pkg/rhttp" @@ -35,6 +29,11 @@ import ( "github.com/mitchellh/mapstructure" "github.com/pkg/errors" "github.com/rs/zerolog" + "io" + "net/http" + "net/url" + "path" + "time" ) const ( diff --git a/internal/http/services/owncloud/ocdav/copy.go b/internal/http/services/owncloud/ocdav/copy.go index 7f14957d37..845c4a165a 100644 --- a/internal/http/services/owncloud/ocdav/copy.go +++ b/internal/http/services/owncloud/ocdav/copy.go @@ -21,12 +21,6 @@ package ocdav import ( "context" "fmt" - "io" - "net/http" - "path" - "strings" - "time" - gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" @@ -37,6 +31,10 @@ import ( tokenpkg "github.com/cs3org/reva/pkg/token" "github.com/eventials/go-tus" "github.com/eventials/go-tus/memorystore" + "io" + "net/http" + "path" + "strings" ) func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) { @@ -282,11 +280,7 @@ func (s *svc) descend(ctx context.Context, client gateway.GatewayAPIClient, src } httpDownloadReq.Header.Set(datagateway.TokenTransportHeader, dRes.Token) - httpDownloadClient := rhttp.GetHTTPClient( - rhttp.Context(ctx), - rhttp.Timeout(time.Duration(s.c.Timeout*int64(time.Second))), - rhttp.Insecure(s.c.Insecure), - ) + httpDownloadClient := s.client httpDownloadRes, err := httpDownloadClient.Do(httpDownloadReq) if err != nil { @@ -314,11 +308,7 @@ func (s *svc) tusUpload(ctx context.Context, dataServerURL string, transferToken // create the tus client. c := tus.DefaultConfig() c.Resume = true - c.HttpClient = rhttp.GetHTTPClient( - rhttp.Context(ctx), - rhttp.Timeout(time.Duration(s.c.Timeout*int64(time.Second))), - rhttp.Insecure(s.c.Insecure), - ) + c.HttpClient = s.client c.Store, err = memorystore.NewMemoryStore() if err != nil { return err diff --git a/internal/http/services/owncloud/ocdav/get.go b/internal/http/services/owncloud/ocdav/get.go index 2d12ece45c..3571223600 100644 --- a/internal/http/services/owncloud/ocdav/get.go +++ b/internal/http/services/owncloud/ocdav/get.go @@ -121,11 +121,7 @@ func (s *svc) handleGet(w http.ResponseWriter, r *http.Request, ns string) { return } httpReq.Header.Set(datagateway.TokenTransportHeader, dRes.Token) - httpClient := rhttp.GetHTTPClient( - rhttp.Context(ctx), - rhttp.Timeout(time.Duration(s.c.Timeout*int64(time.Second))), - rhttp.Insecure(s.c.Insecure), - ) + httpClient := s.client httpRes, err := httpClient.Do(httpReq) if err != nil { diff --git a/internal/http/services/owncloud/ocdav/ocdav.go b/internal/http/services/owncloud/ocdav/ocdav.go index f1c964d5b7..405e3e99ee 100644 --- a/internal/http/services/owncloud/ocdav/ocdav.go +++ b/internal/http/services/owncloud/ocdav/ocdav.go @@ -22,16 +22,11 @@ import ( "context" "encoding/base64" "fmt" - "net/http" - "net/url" - "os" - "path" - "strings" - gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/cs3org/reva/pkg/appctx" "github.com/cs3org/reva/pkg/rgrpc/todo/pool" + "github.com/cs3org/reva/pkg/rhttp" "github.com/cs3org/reva/pkg/rhttp/global" "github.com/cs3org/reva/pkg/rhttp/router" "github.com/cs3org/reva/pkg/sharedconf" @@ -40,6 +35,12 @@ import ( "github.com/mitchellh/mapstructure" "github.com/pkg/errors" "github.com/rs/zerolog" + "net/http" + "net/url" + "os" + "path" + "strings" + "time" ) type ctxKey int @@ -87,6 +88,7 @@ type svc struct { c *Config webDavHandler *WebDavHandler davHandler *DavHandler + client *http.Client } // New returns a new ocdav @@ -106,6 +108,10 @@ func New(m map[string]interface{}, log *zerolog.Logger) (global.Service, error) c: conf, webDavHandler: new(WebDavHandler), davHandler: new(DavHandler), + client: rhttp.GetHTTPClient( + rhttp.Timeout(time.Duration(conf.Timeout*int64(time.Second))), + rhttp.Insecure(conf.Insecure), + ), } // initialize handlers and set default configs if err := s.webDavHandler.init(conf.WebdavNamespace); err != nil { diff --git a/internal/http/services/owncloud/ocdav/put.go b/internal/http/services/owncloud/ocdav/put.go index 51fae4d9a7..4310d8789c 100644 --- a/internal/http/services/owncloud/ocdav/put.go +++ b/internal/http/services/owncloud/ocdav/put.go @@ -31,7 +31,6 @@ import ( "github.com/cs3org/reva/internal/http/services/datagateway" "github.com/cs3org/reva/internal/http/utils" "github.com/cs3org/reva/pkg/appctx" - "github.com/cs3org/reva/pkg/rhttp" tokenpkg "github.com/cs3org/reva/pkg/token" "github.com/eventials/go-tus" "github.com/eventials/go-tus/memorystore" @@ -272,11 +271,8 @@ func (s *svc) handlePut(w http.ResponseWriter, r *http.Request, ns string) { // create the tus client. c := tus.DefaultConfig() c.Resume = true - c.HttpClient = rhttp.GetHTTPClient( - rhttp.Context(ctx), - rhttp.Timeout(time.Duration(s.c.Timeout*int64(time.Second))), - rhttp.Insecure(s.c.Insecure), - ) + c.HttpClient = s.client + c.Store, err = memorystore.NewMemoryStore() if err != nil { w.WriteHeader(http.StatusInternalServerError) diff --git a/internal/http/services/owncloud/ocdav/tus.go b/internal/http/services/owncloud/ocdav/tus.go index 5eddefb5d9..e3dc016948 100644 --- a/internal/http/services/owncloud/ocdav/tus.go +++ b/internal/http/services/owncloud/ocdav/tus.go @@ -193,11 +193,7 @@ func (s *svc) handleTusPost(w http.ResponseWriter, r *http.Request, ns string) { var httpRes *http.Response if length != 0 { - httpClient := rhttp.GetHTTPClient( - rhttp.Context(ctx), - rhttp.Timeout(time.Duration(s.c.Timeout*int64(time.Second))), - rhttp.Insecure(s.c.Insecure), - ) + httpClient := s.client httpReq, err := rhttp.NewRequest(ctx, "PATCH", uRes.UploadEndpoint, r.Body) if err != nil { log.Err(err).Msg("wrong request") From c7f4159ab816ce7fd9a1156e08475585dc7e71c7 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 20 Oct 2020 16:30:18 +0200 Subject: [PATCH 2/5] add changelog --- changelog/unreleased/ocdav-fix-file-descriptor-leak.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/unreleased/ocdav-fix-file-descriptor-leak.md diff --git a/changelog/unreleased/ocdav-fix-file-descriptor-leak.md b/changelog/unreleased/ocdav-fix-file-descriptor-leak.md new file mode 100644 index 0000000000..1b064803cd --- /dev/null +++ b/changelog/unreleased/ocdav-fix-file-descriptor-leak.md @@ -0,0 +1,5 @@ +Bugfix: Fix file descriptor leak on ocdav put handler + +File descriptors on the ocdav service, especially on the put handler was leaking http connections. This PR addresses this. + +https://github.com/cs3org/reva/pull/1259 \ No newline at end of file From 3848b36aaf500f26aea713ca25ab209b10c4af56 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 20 Oct 2020 16:33:44 +0200 Subject: [PATCH 3/5] fix pr= --- changelog/unreleased/ocdav-fix-file-descriptor-leak.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/unreleased/ocdav-fix-file-descriptor-leak.md b/changelog/unreleased/ocdav-fix-file-descriptor-leak.md index 1b064803cd..11571b96ce 100644 --- a/changelog/unreleased/ocdav-fix-file-descriptor-leak.md +++ b/changelog/unreleased/ocdav-fix-file-descriptor-leak.md @@ -2,4 +2,4 @@ Bugfix: Fix file descriptor leak on ocdav put handler File descriptors on the ocdav service, especially on the put handler was leaking http connections. This PR addresses this. -https://github.com/cs3org/reva/pull/1259 \ No newline at end of file +https://github.com/cs3org/reva/pull/1260 \ No newline at end of file From 46c092a690090210d3487560f3b54220940aee78 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 20 Oct 2020 16:39:16 +0200 Subject: [PATCH 4/5] run goimports --- internal/http/services/datagateway/datagateway.go | 11 ++++++----- internal/http/services/owncloud/ocdav/copy.go | 9 +++++---- internal/http/services/owncloud/ocdav/ocdav.go | 15 ++++++++------- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/internal/http/services/datagateway/datagateway.go b/internal/http/services/datagateway/datagateway.go index 04b58499cb..7b7549f84b 100644 --- a/internal/http/services/datagateway/datagateway.go +++ b/internal/http/services/datagateway/datagateway.go @@ -20,6 +20,12 @@ package datagateway import ( "context" + "io" + "net/http" + "net/url" + "path" + "time" + "github.com/cs3org/reva/pkg/appctx" "github.com/cs3org/reva/pkg/errtypes" "github.com/cs3org/reva/pkg/rhttp" @@ -29,11 +35,6 @@ import ( "github.com/mitchellh/mapstructure" "github.com/pkg/errors" "github.com/rs/zerolog" - "io" - "net/http" - "net/url" - "path" - "time" ) const ( diff --git a/internal/http/services/owncloud/ocdav/copy.go b/internal/http/services/owncloud/ocdav/copy.go index 845c4a165a..032ff70841 100644 --- a/internal/http/services/owncloud/ocdav/copy.go +++ b/internal/http/services/owncloud/ocdav/copy.go @@ -21,6 +21,11 @@ package ocdav import ( "context" "fmt" + "io" + "net/http" + "path" + "strings" + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" rpc "github.com/cs3org/go-cs3apis/cs3/rpc/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" @@ -31,10 +36,6 @@ import ( tokenpkg "github.com/cs3org/reva/pkg/token" "github.com/eventials/go-tus" "github.com/eventials/go-tus/memorystore" - "io" - "net/http" - "path" - "strings" ) func (s *svc) handleCopy(w http.ResponseWriter, r *http.Request, ns string) { diff --git a/internal/http/services/owncloud/ocdav/ocdav.go b/internal/http/services/owncloud/ocdav/ocdav.go index 405e3e99ee..f34e37a340 100644 --- a/internal/http/services/owncloud/ocdav/ocdav.go +++ b/internal/http/services/owncloud/ocdav/ocdav.go @@ -22,6 +22,13 @@ import ( "context" "encoding/base64" "fmt" + "net/http" + "net/url" + "os" + "path" + "strings" + "time" + gateway "github.com/cs3org/go-cs3apis/cs3/gateway/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/cs3org/reva/pkg/appctx" @@ -35,12 +42,6 @@ import ( "github.com/mitchellh/mapstructure" "github.com/pkg/errors" "github.com/rs/zerolog" - "net/http" - "net/url" - "os" - "path" - "strings" - "time" ) type ctxKey int @@ -88,7 +89,7 @@ type svc struct { c *Config webDavHandler *WebDavHandler davHandler *DavHandler - client *http.Client + client *http.Client } // New returns a new ocdav From fc0be0a7118182c319d76ccf85294ded483b5a04 Mon Sep 17 00:00:00 2001 From: "A.Unger" Date: Tue, 20 Oct 2020 16:52:55 +0200 Subject: [PATCH 5/5] reuse clients on datagateway --- .../http/services/datagateway/datagateway.go | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/internal/http/services/datagateway/datagateway.go b/internal/http/services/datagateway/datagateway.go index 7b7549f84b..64e92dcd35 100644 --- a/internal/http/services/datagateway/datagateway.go +++ b/internal/http/services/datagateway/datagateway.go @@ -69,6 +69,7 @@ func (c *config) init() { type svc struct { conf *config handler http.Handler + client *http.Client } // New returns a new datagateway @@ -80,7 +81,13 @@ func New(m map[string]interface{}, log *zerolog.Logger) (global.Service, error) conf.init() - s := &svc{conf: conf} + s := &svc{ + conf: conf, + client: rhttp.GetHTTPClient( + rhttp.Timeout(time.Duration(conf.Timeout*int64(time.Second))), + rhttp.Insecure(conf.Insecure), + ), + } s.setHandler() return s, nil } @@ -170,11 +177,7 @@ func (s *svc) doHead(w http.ResponseWriter, r *http.Request) { log.Debug().Str("target", claims.Target).Msg("sending request to internal data server") - httpClient := rhttp.GetHTTPClient( - rhttp.Context(ctx), - rhttp.Timeout(time.Duration(s.conf.Timeout*int64(time.Second))), - rhttp.Insecure(s.conf.Insecure), - ) + httpClient := s.client httpReq, err := rhttp.NewRequest(ctx, "HEAD", claims.Target, nil) if err != nil { log.Err(err).Msg("wrong request") @@ -215,11 +218,7 @@ func (s *svc) doGet(w http.ResponseWriter, r *http.Request) { log.Debug().Str("target", claims.Target).Msg("sending request to internal data server") - httpClient := rhttp.GetHTTPClient( - rhttp.Context(ctx), - rhttp.Timeout(time.Duration(s.conf.Timeout*int64(time.Second))), - rhttp.Insecure(s.conf.Insecure), - ) + httpClient := s.client httpReq, err := rhttp.NewRequest(ctx, "GET", claims.Target, nil) if err != nil { log.Err(err).Msg("wrong request") @@ -275,11 +274,7 @@ func (s *svc) doPut(w http.ResponseWriter, r *http.Request) { log.Debug().Str("target", claims.Target).Msg("sending request to internal data server") - httpClient := rhttp.GetHTTPClient( - rhttp.Context(ctx), - rhttp.Timeout(time.Duration(s.conf.Timeout*int64(time.Second))), - rhttp.Insecure(s.conf.Insecure), - ) + httpClient := s.client httpReq, err := rhttp.NewRequest(ctx, "PUT", target, r.Body) if err != nil { log.Err(err).Msg("wrong request") @@ -336,11 +331,7 @@ func (s *svc) doPatch(w http.ResponseWriter, r *http.Request) { log.Debug().Str("target", claims.Target).Msg("sending request to internal data server") - httpClient := rhttp.GetHTTPClient( - rhttp.Context(ctx), - rhttp.Timeout(time.Duration(s.conf.Timeout*int64(time.Second))), - rhttp.Insecure(s.conf.Insecure), - ) + httpClient := s.client httpReq, err := rhttp.NewRequest(ctx, "PATCH", target, r.Body) if err != nil { log.Err(err).Msg("wrong request")