labstack
Echo response object calls flush on unflushable objects
Issue Description
The Echo Response object causes a panic when calling the Flush method with an unflushable parent writer.
For example, if you are using TimeoutMiddleware, which creates TimeoutHandler (github.com/labstack/echo/v4/middleware/timeout.go:124) and then creates a timeout writer (src/net/http/server.go:3584), and you try to call the Flush method in the echo.Context object, it will result in a panic. This happens because it doesn't check if the Writer implements the Flush method before calling it (github.com/labstack/echo/v4/response.go:87). This issue can be handled similarly to how the standard http library does it (httputil/reverseproxy.go:524).
I've noticed duplicated issues, but in my humble opinion, you should try to unpack writers and search for flushable ones, similar to how it's done in the standard library. What do you think?
Checklist
- [X] Dependencies installed
- [X] No typos
- [X] Searched existing issues and docs
Expected behaviour
Calling Flush method with response writers that doesn't implements Flusher interface should not panicking
Actual behaviour
Calling Flush method with response writers that doesn't implements Flusher interface is panicking
Steps to reproduce
- Use timeout middleware
- Implement endpoint that call c.(echo.Context).Response().Flush() method
- Curl that endpoint
Working code to debug
func main() {
e := echo.New()
e.Use(middleware.TimeoutWithConfig(middleware.TimeoutConfig{
Timeout: 3 * time.Second,
}))
e.GET("/ping", func(c echo.Context) error {
c.Response().Flush()
return c.String(http.StatusOK, "pong")
})
if err := e.Start(":8080"); err != nil {
log.Fatalln(err.Error())
}
}
Version/commit
github.com/labstack/echo/v4 v4.11.4
e.GET("/ping", func(c echo.Context) error {
c.Response().Flush()
return c.String(http.StatusOK, "pong")
})
Is not very good example. Or lets rephrase that - this shows that Timeout middleware has problems with handling requests that want to flush the response.
We could change Response.Flush() to support unwrapping but I think we still should panic if unwrapping ends up with http.ErrNotSupported. Hiding the fact that Flush did not do anything hides potential problems. In that example we have explicit call for Flush which means that there is some requirement why we need to flush. No-oping that could be a bug.
https://github.com/labstack/echo/blob/29aab274b3810dfd4e1be172d5a569ac3b9efcd6/response.go#L86
to
func (r *Response) Flush() {
err := http.NewResponseController(r.Writer).Flush()
if err != nil && errors.Is(err, http.ErrNotSupported) {
panic(fmt.Errorf("response writer flushing is not supported"))
}
}
and make Timeout middleware support unwrapping (and other custom writers we have created)
NB: you should avoid using Timeout middleware. In its basic form it just sends the response to the client and potentially does not end your handler goroutine if you have not properly implemented context cancellation checks.
If you want to properly handle timeout you need to implement context checks and this renders Timeout MW useless. As then you check for context.DeadlineExceeded/context.Canceled and know it is reached you will can send the response to client.
I agree that it's supposed to cause panic if flushing is not supported, but at least, it should have a chance to unwrap parent writers to check if there is a flusher one.
About timeout middleware, I don't fully understand you, It does just what I want it to do - cancel context, if timeout is reached
I'll create PR for https://github.com/labstack/echo/issues/2592#issuecomment-1939599287 changes
@qerdcv in you example you are using middleware.TimeoutWithConfig this uses goroutine to serve your request. Maybe you wanted to use middleware.ContextTimeout() ?
- ContextTimeout just replaces context for request https://github.com/labstack/echo/blob/ea529bbab6602db8bd9fc0746405a3687ffbd885/middleware/context_timeout.go#L61
- TimeoutWithConfig runs your request in separate goroutine https://github.com/labstack/echo/blob/ea529bbab6602db8bd9fc0746405a3687ffbd885/middleware/timeout.go#L124 https://github.com/golang/go/blob/e17e5308fd5a26da5702d16cc837ee77cdb30ab6/src/net/http/server.go#L3590-L3598
There are recommendations for timeout handling at the start of https://github.com/labstack/echo/blob/ea529bbab6602db8bd9fc0746405a3687ffbd885/middleware/timeout.go
It seems that you are right. I'll recheck what behaviour exactly I need. Thanks!