Add context timeout middleware

[hakankutluay]

We need to introduce a new middleware (middleware.ContextTimeout()) that creates context with timeout and injects ContextWithTimeout to c.Request().Context(). If the handler returns an error that wraps context.DeadlineExceeded, it returns Service Unavailable (503)

This fixes #2379, #2306.

Co-authored-by: @erhanakp

[aldas]

I think this error handling part is too rigid. It should be left to developer to decide.

Also err = c.Request().Context().Err() should not be needed as then context timeouts your timeout aware methods should return that error. unless you are warpping somewhere you should not need to check it from request.

and middlewares should not handle errors like that return c.JSON(http.StatusServiceUnavailable, suResponse)

this means that now your middleware "commits" that response and midlewares up in stack can not handle errors anymore

I propose something like that instead:

type ContextTimeoutConfig struct { Skipper middleware.Skipper ErrorHandler func(err error, c echo.Context) error Timeout time.Duration } func ContextTimeout(timeout time.Duration) echo.MiddlewareFunc { return ContextTimeoutWithConfig(ContextTimeoutConfig{Timeout: timeout}) } func ContextTimeoutWithConfig(config ContextTimeoutConfig) echo.MiddlewareFunc { return config.ToMiddleware() } func (config ContextTimeoutConfig) ToMiddleware() echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { return func(c echo.Context) error { if config.Skipper(c) || config.Timeout == 0 { return next(c)	} timeoutContext, cancel := context.WithTimeout(c.Request().Context(), config.Timeout) defer cancel() c.SetRequest(c.Request().WithContext(timeoutContext)) err := next(c) if err != nil && config.ErrorHandler != nil { return config.ErrorHandler(err, c)	} return err	}	} }

[hakankutluay]

Thanks @aldas for your valuable feedback and comments.

Would you please review the updated PR?

[hakankutluay]

@aldas, thanks for your comments. all change requests are completed.

[codecov]

Codecov Report

Base: 92.75% // Head: 92.80% // Increases project coverage by +0.05% 🎉

Coverage data is based on head (0f8434b) compared to base (24a3061).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@ Coverage Diff @@ ## master #2380 +/- ## ========================================== + Coverage 92.75% 92.80% +0.05%  ========================================== Files 37 38 +1 Lines 4473 4506 +33 ========================================== + Hits 4149 4182 +33  Misses 236 236 Partials 88 88 

Impacted Files	Coverage Δ
middleware/context_timeout.go	100.00% <100.00%> (ø)
router.go	97.79% <0.00%> (-0.01%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[aldas]

coverage.coverprofile file is added here by accident with latest commit and should be removed

[erhanakp]

coverage.coverprofile file is added here by accident with latest commit and should be removed @aldas

👍 removed

[aldas]

LGTM

[hakankutluay]

@aldas timeout durations increased to fix tests.

[aldas]

LGTM

[HeCorr]

Just checking, was #2306 meant to be auto-closed with the merge of this? Because it's still open.

[aldas]

@HeCorr , no idea. that issue is probably long running file download wrapped by timeout, that does not finish by the time context gets reused. but this is a wild guess as there are no meaningful information

[HeCorr]

@aldas I asked because it seems @hakankutluay meant to close it together with #2379 by the way he wrote it:

but the fixes/closes keywords don't seem to support a comma-separated list of issues, so only the first one was closed.

[aldas]

I closed #2306 this MW is answer for some of the problems there.