[ARVADOS] created: 2.1.0-1389-gfdd3dd304
Git user
git at public.arvados.org
Thu Sep 23 15:12:38 UTC 2021
at fdd3dd304439a7912db1c6ef257cf512ec7b3b52 (commit)
commit fdd3dd304439a7912db1c6ef257cf512ec7b3b52
Author: Tom Clegg <tom at curii.com>
Date: Thu Sep 23 10:46:42 2021 -0400
13697: Do not apply RequestTimeout to hijacked connections.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/sdk/go/httpserver/logger.go b/sdk/go/httpserver/logger.go
index a0ca6bf28..7eb7f0f03 100644
--- a/sdk/go/httpserver/logger.go
+++ b/sdk/go/httpserver/logger.go
@@ -5,7 +5,9 @@
package httpserver
import (
+ "bufio"
"context"
+ "net"
"net/http"
"time"
@@ -22,12 +24,42 @@ var (
requestTimeContextKey = contextKey{"requestTime"}
)
+type hijacker interface {
+ http.ResponseWriter
+ http.Hijacker
+}
+
+// hijackNotifier wraps a ResponseWriter, calling the provided
+// Notify() func if/when the wrapped Hijacker is hijacked.
+type hijackNotifier struct {
+ hijacker
+ hijacked chan<- bool
+}
+
+func (hn hijackNotifier) Hijack() (net.Conn, *bufio.ReadWriter, error) {
+ close(hn.hijacked)
+ return hn.hijacker.Hijack()
+}
+
// HandlerWithDeadline cancels the request context if the request
-// takes longer than the specified timeout.
+// takes longer than the specified timeout without having its
+// connection hijacked.
func HandlerWithDeadline(timeout time.Duration, next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- ctx, cancel := context.WithDeadline(r.Context(), time.Now().Add(timeout))
+ ctx, cancel := context.WithCancel(r.Context())
defer cancel()
+ nodeadline := make(chan bool)
+ go func() {
+ select {
+ case <-nodeadline:
+ case <-ctx.Done():
+ case <-time.After(timeout):
+ cancel()
+ }
+ }()
+ if hj, ok := w.(hijacker); ok {
+ w = hijackNotifier{hj, nodeadline}
+ }
next.ServeHTTP(w, r.WithContext(ctx))
})
}
diff --git a/sdk/go/httpserver/logger_test.go b/sdk/go/httpserver/logger_test.go
index b623aa4ee..60768b3fc 100644
--- a/sdk/go/httpserver/logger_test.go
+++ b/sdk/go/httpserver/logger_test.go
@@ -9,6 +9,8 @@ import (
"context"
"encoding/json"
"fmt"
+ "io/ioutil"
+ "net"
"net/http"
"net/http/httptest"
"testing"
@@ -70,6 +72,32 @@ func (s *Suite) TestWithDeadline(c *check.C) {
c.Check(resp.Body.String(), check.Equals, "ok")
}
+func (s *Suite) TestNoDeadlineAfterHijacked(c *check.C) {
+ srv := Server{
+ Addr: ":",
+ Server: http.Server{
+ Handler: HandlerWithDeadline(time.Millisecond, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+ conn, _, err := w.(http.Hijacker).Hijack()
+ c.Assert(err, check.IsNil)
+ defer conn.Close()
+ select {
+ case <-req.Context().Done():
+ c.Error("request context done too soon")
+ case <-time.After(time.Second / 10):
+ conn.Write([]byte("HTTP/1.1 200 OK\r\n\r\nok"))
+ }
+ })),
+ BaseContext: func(net.Listener) context.Context { return s.ctx },
+ },
+ }
+ srv.Start()
+ defer srv.Close()
+ resp, err := http.Get("http://" + srv.Addr)
+ c.Assert(err, check.IsNil)
+ body, err := ioutil.ReadAll(resp.Body)
+ c.Check(string(body), check.Equals, "ok")
+}
+
func (s *Suite) TestLogRequests(c *check.C) {
h := AddRequestIDs(LogRequests(
http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
commit fc1c9a3d80a5fc71142ad11f4eda05c26e351569
Author: Tom Clegg <tom at curii.com>
Date: Thu Sep 23 09:44:29 2021 -0400
13697: Remove HandlerWithContext.
Use http.Server's BaseContext feature instead.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/lib/controller/auth_test.go b/lib/controller/auth_test.go
index 69458655b..175241146 100644
--- a/lib/controller/auth_test.go
+++ b/lib/controller/auth_test.go
@@ -8,6 +8,7 @@ import (
"context"
"encoding/json"
"fmt"
+ "net"
"net/http"
"net/http/httptest"
"os"
@@ -99,9 +100,10 @@ func (s *AuthSuite) SetUpTest(c *check.C) {
s.testHandler = &Handler{Cluster: cluster}
s.testServer = newServerFromIntegrationTestEnv(c)
- s.testServer.Server.Handler = httpserver.HandlerWithContext(
- ctxlog.Context(context.Background(), s.log),
- httpserver.AddRequestIDs(httpserver.LogRequests(s.testHandler)))
+ s.testServer.Server.BaseContext = func(net.Listener) context.Context {
+ return ctxlog.Context(context.Background(), s.log)
+ }
+ s.testServer.Server.Handler = httpserver.AddRequestIDs(httpserver.LogRequests(s.testHandler))
c.Assert(s.testServer.Start(), check.IsNil)
}
diff --git a/lib/controller/federation_test.go b/lib/controller/federation_test.go
index 6d74ab65f..211c76198 100644
--- a/lib/controller/federation_test.go
+++ b/lib/controller/federation_test.go
@@ -11,6 +11,7 @@ import (
"fmt"
"io"
"io/ioutil"
+ "net"
"net/http"
"net/http/httptest"
"net/url"
@@ -71,9 +72,10 @@ func (s *FederationSuite) SetUpTest(c *check.C) {
arvadostest.SetServiceURL(&cluster.Services.Controller, "http://localhost:/")
s.testHandler = &Handler{Cluster: cluster}
s.testServer = newServerFromIntegrationTestEnv(c)
- s.testServer.Server.Handler = httpserver.HandlerWithContext(
- ctxlog.Context(context.Background(), s.log),
- httpserver.AddRequestIDs(httpserver.LogRequests(s.testHandler)))
+ s.testServer.Server.BaseContext = func(net.Listener) context.Context {
+ return ctxlog.Context(context.Background(), s.log)
+ }
+ s.testServer.Server.Handler = httpserver.AddRequestIDs(httpserver.LogRequests(s.testHandler))
cluster.RemoteClusters = map[string]arvados.RemoteCluster{
"zzzzz": {
diff --git a/lib/controller/server_test.go b/lib/controller/server_test.go
index 051f35571..b2b3365a2 100644
--- a/lib/controller/server_test.go
+++ b/lib/controller/server_test.go
@@ -6,6 +6,7 @@ package controller
import (
"context"
+ "net"
"net/http"
"os"
"path/filepath"
@@ -48,9 +49,10 @@ func newServerFromIntegrationTestEnv(c *check.C) *httpserver.Server {
srv := &httpserver.Server{
Server: http.Server{
- Handler: httpserver.HandlerWithContext(
- ctxlog.Context(context.Background(), log),
- httpserver.AddRequestIDs(httpserver.LogRequests(handler))),
+ BaseContext: func(net.Listener) context.Context {
+ return ctxlog.Context(context.Background(), log)
+ },
+ Handler: httpserver.AddRequestIDs(httpserver.LogRequests(handler)),
},
Addr: ":",
}
diff --git a/sdk/go/httpserver/logger.go b/sdk/go/httpserver/logger.go
index 191688096..a0ca6bf28 100644
--- a/sdk/go/httpserver/logger.go
+++ b/sdk/go/httpserver/logger.go
@@ -22,15 +22,6 @@ var (
requestTimeContextKey = contextKey{"requestTime"}
)
-// HandlerWithContext returns an http.Handler that changes the request
-// context to ctx (replacing http.Server's default
-// context.Background()), then calls next.
-func HandlerWithContext(ctx context.Context, next http.Handler) http.Handler {
- return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- next.ServeHTTP(w, r.WithContext(ctx))
- })
-}
-
// HandlerWithDeadline cancels the request context if the request
// takes longer than the specified timeout.
func HandlerWithDeadline(timeout time.Duration, next http.Handler) http.Handler {
diff --git a/services/keep-web/server.go b/services/keep-web/server.go
index 8f623c627..b0375ff8d 100644
--- a/services/keep-web/server.go
+++ b/services/keep-web/server.go
@@ -6,6 +6,7 @@ package main
import (
"context"
+ "net"
"net/http"
"git.arvados.org/arvados.git/sdk/go/arvados"
@@ -24,10 +25,10 @@ func (srv *server) Start(logger *logrus.Logger) error {
h := &handler{Config: srv.Config}
reg := prometheus.NewRegistry()
h.Config.Cache.registry = reg
- ctx := ctxlog.Context(context.Background(), logger)
- mh := httpserver.Instrument(reg, logger, httpserver.HandlerWithContext(ctx, httpserver.AddRequestIDs(httpserver.LogRequests(h))))
+ mh := httpserver.Instrument(reg, logger, httpserver.AddRequestIDs(httpserver.LogRequests(h)))
h.MetricsAPI = mh.ServeAPI(h.Config.cluster.ManagementToken, http.NotFoundHandler())
srv.Handler = mh
+ srv.BaseContext = func(net.Listener) context.Context { return ctxlog.Context(context.Background(), logger) }
var listen arvados.URL
for listen = range srv.Config.cluster.Services.WebDAV.InternalURLs {
break
commit 2b8fc576e242c0b8658eef9f1130143e009efc4d
Author: Tom Clegg <tom at curii.com>
Date: Thu Sep 23 09:41:20 2021 -0400
13697: Cancel request context after API.RequestTimeout.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/lib/service/cmd.go b/lib/service/cmd.go
index 40db4f9c7..e67c24f65 100644
--- a/lib/service/cmd.go
+++ b/lib/service/cmd.go
@@ -126,13 +126,14 @@ func (c *command) RunCommand(prog string, args []string, stdin io.Reader, stdout
}
instrumented := httpserver.Instrument(reg, log,
- httpserver.HandlerWithContext(ctx,
+ httpserver.HandlerWithDeadline(cluster.API.RequestTimeout.Duration(),
httpserver.AddRequestIDs(
httpserver.LogRequests(
httpserver.NewRequestLimiter(cluster.API.MaxConcurrentRequests, handler, reg)))))
srv := &httpserver.Server{
Server: http.Server{
- Handler: instrumented.ServeAPI(cluster.ManagementToken, instrumented),
+ Handler: instrumented.ServeAPI(cluster.ManagementToken, instrumented),
+ BaseContext: func(net.Listener) context.Context { return ctx },
},
Addr: listenURL.Host,
}
diff --git a/sdk/go/httpserver/logger.go b/sdk/go/httpserver/logger.go
index 78a1f77ad..191688096 100644
--- a/sdk/go/httpserver/logger.go
+++ b/sdk/go/httpserver/logger.go
@@ -31,6 +31,16 @@ func HandlerWithContext(ctx context.Context, next http.Handler) http.Handler {
})
}
+// HandlerWithDeadline cancels the request context if the request
+// takes longer than the specified timeout.
+func HandlerWithDeadline(timeout time.Duration, next http.Handler) http.Handler {
+ return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ ctx, cancel := context.WithDeadline(r.Context(), time.Now().Add(timeout))
+ defer cancel()
+ next.ServeHTTP(w, r.WithContext(ctx))
+ })
+}
+
// LogRequests wraps an http.Handler, logging each request and
// response.
func LogRequests(h http.Handler) http.Handler {
diff --git a/sdk/go/httpserver/logger_test.go b/sdk/go/httpserver/logger_test.go
index af45a640c..b623aa4ee 100644
--- a/sdk/go/httpserver/logger_test.go
+++ b/sdk/go/httpserver/logger_test.go
@@ -41,6 +41,35 @@ func (s *Suite) SetUpTest(c *check.C) {
s.ctx = ctxlog.Context(context.Background(), s.log)
}
+func (s *Suite) TestWithDeadline(c *check.C) {
+ req, err := http.NewRequest("GET", "https://foo.example/bar", nil)
+ c.Assert(err, check.IsNil)
+
+ // Short timeout cancels context in <1s
+ resp := httptest.NewRecorder()
+ HandlerWithDeadline(time.Millisecond, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+ select {
+ case <-req.Context().Done():
+ w.Write([]byte("ok"))
+ case <-time.After(time.Second):
+ c.Error("timed out")
+ }
+ })).ServeHTTP(resp, req.WithContext(s.ctx))
+ c.Check(resp.Body.String(), check.Equals, "ok")
+
+ // Long timeout does not cancel context in <1ms
+ resp = httptest.NewRecorder()
+ HandlerWithDeadline(time.Second, http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+ select {
+ case <-req.Context().Done():
+ c.Error("request context done too soon")
+ case <-time.After(time.Millisecond):
+ w.Write([]byte("ok"))
+ }
+ })).ServeHTTP(resp, req.WithContext(s.ctx))
+ c.Check(resp.Body.String(), check.Equals, "ok")
+}
+
func (s *Suite) TestLogRequests(c *check.C) {
h := AddRequestIDs(LogRequests(
http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
@@ -52,7 +81,7 @@ func (s *Suite) TestLogRequests(c *check.C) {
c.Assert(err, check.IsNil)
resp := httptest.NewRecorder()
- HandlerWithContext(s.ctx, h).ServeHTTP(resp, req)
+ h.ServeHTTP(resp, req.WithContext(s.ctx))
dec := json.NewDecoder(s.logdata)
@@ -104,12 +133,12 @@ func (s *Suite) TestLogErrorBody(c *check.C) {
c.Assert(err, check.IsNil)
resp := httptest.NewRecorder()
- HandlerWithContext(s.ctx, LogRequests(
+ LogRequests(
http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
w.WriteHeader(trial.statusCode)
w.Write([]byte(trial.sentBody))
}),
- )).ServeHTTP(resp, req)
+ ).ServeHTTP(resp, req.WithContext(s.ctx))
gotReq := make(map[string]interface{})
err = dec.Decode(&gotReq)
commit d90f02c0e969b649da61fdd272a9aab69c487de3
Author: Tom Clegg <tom at curii.com>
Date: Wed Sep 22 16:57:57 2021 -0400
13697: Set Rails database timeout to API.RequestTimeout config.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/config/arvados_config.rb b/services/api/config/arvados_config.rb
index ea421a289..49865039c 100644
--- a/services/api/config/arvados_config.rb
+++ b/services/api/config/arvados_config.rb
@@ -84,6 +84,7 @@ arvcfg.declare_config "API.MaxRequestSize", Integer, :max_request_size
arvcfg.declare_config "API.MaxIndexDatabaseRead", Integer, :max_index_database_read
arvcfg.declare_config "API.MaxItemsPerResponse", Integer, :max_items_per_response
arvcfg.declare_config "API.MaxTokenLifetime", ActiveSupport::Duration
+arvcfg.declare_config "API.RequestTimeout", ActiveSupport::Duration
arvcfg.declare_config "API.AsyncPermissionsUpdateInterval", ActiveSupport::Duration, :async_permissions_update_interval
arvcfg.declare_config "Users.AutoSetupNewUsers", Boolean, :auto_setup_new_users
arvcfg.declare_config "Users.AutoSetupNewUsersWithVmUUID", String, :auto_setup_new_users_with_vm_uuid
diff --git a/services/api/config/initializers/db_timeout.rb b/services/api/config/initializers/db_timeout.rb
new file mode 100644
index 000000000..3b61f15c8
--- /dev/null
+++ b/services/api/config/initializers/db_timeout.rb
@@ -0,0 +1,9 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+ActiveRecord::ConnectionAdapters::AbstractAdapter.set_callback :checkout, :before, ->(conn) do
+ ms = Rails.configuration.API.RequestTimeout.to_i * 1000
+ conn.execute("SET statement_timeout = #{ms}")
+ conn.execute("SET lock_timeout = #{ms}")
+end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list