Data Race in timeout middleware #1761
Comments
|
Snippet of data race error output: |
|
Interesting bug and easily reproducable. Not that easy to fix though. |
Run with `go test -race -count 1 -timeout 10s -run ^TestTimeoutDataRace$ github.com/labstack/echo/v4/middleware`
|
If we want to keep behavior ... that is:
Problem is that we have 2 coroutines (original coroutine created by e.ServeHTTP listener loop and second one created in timeout middleware) and share same response writer in between those two. we would need to use Actually. lets go back a little and think what we want to achieve. One of the original issues has this requirement
In my understanding this wording does no imply that original issue creator wanted to return early to requestor and let handler execute. Also there is // TimeoutHandler returns a Handler that runs h with the given time limit.
//
// The new Handler calls h.ServeHTTP to handle each request, but if a
// call runs for longer than its time limit, the handler responds with
// a 503 Service Unavailable error and the given message in its body.
// (If msg is empty, a suitable default message will be sent.)
// After such a timeout, writes by h to its ResponseWriter will return
// ErrHandlerTimeout.
//
// TimeoutHandler supports the Pusher interface but does not support
// the Hijacker or Flusher interfaces.
func TimeoutHandler(h Handler, dt time.Duration, msg string) Handler { |
|
Something like that func TimeoutWithConfig(config TimeoutConfig) echo.MiddlewareFunc {
// Defaults
if config.Skipper == nil {
config.Skipper = DefaultTimeoutConfig.Skipper
}
return func(next echo.HandlerFunc) echo.HandlerFunc {
return func(c echo.Context) error {
if config.Skipper(c) || config.Timeout == 0 {
return next(c)
}
handlerWrapper := echoHandlerFuncWrapper{
ctx: c,
handler: next,
errChan: make(chan error, 1),
}
handler := http.TimeoutHandler(handlerWrapper, config.Timeout, config.ErrorMessage)
handler.ServeHTTP(c.Response().Writer, c.Request())
select {
case err := <-handlerWrapper.errChan:
return err
default:
return nil
}
}
}
}
type echoHandlerFuncWrapper struct {
ctx echo.Context
handler echo.HandlerFunc
errChan chan error
}
func (t echoHandlerFuncWrapper) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
// replace writer with TimeoutHandler custom one. This will guarantee that
// `writes by h to its ResponseWriter will return ErrHandlerTimeout.`
originalWriter := t.ctx.Response().Writer
t.ctx.Response().Writer = rw
err := t.handler(t.ctx)
t.ctx.Response().Writer = originalWriter
if ctxErr := r.Context().Err(); ctxErr == context.DeadlineExceeded {
return // on timeout we can not send handler error to client because `http.TimeoutHandler` has already sent headers
}
if err != nil {
t.errChan <- err
}
}func main() {
e := echo.New()
e.Use(middleware.Logger())
e.Use(middleware.TimeoutWithConfig(middleware.TimeoutConfig{
Timeout: 1 * time.Second,
}))
e.GET("/", func(c echo.Context) error {
time.Sleep(2 * time.Second)
return c.String(http.StatusOK, "OK")
//return echo.NewHTTPError(http.StatusForbidden, "nope")
})
e.Start(":8080")
}x@x:~/code/echo$ time curl http://localhost:8080
Timeout! change me
real 0m1.012s
user 0m0.008s
sys 0m0.005s
|
…Replace `ErrorHandler` with `ErrorMessage` on config struct (fix labstack#1761)
Issue Description
Timeout middleware added in #1684 fails data race detector when the handler tries to write to the response at same time as the context times out.
Checklist
Expected behaviour
There should be no data race when writing the response in the downstream handler.
Actual behaviour
A data race is detected when writing the response at roughly the same time as the middleware times out.
Steps to reproduce
Working code to debug
execute with
to see errors from data race detector.
Version/commit
7c8592a
The text was updated successfully, but these errors were encountered: