From 45b08e1da4750d9e6304be82b92057f00bc1f993 Mon Sep 17 00:00:00 2001 From: toimtoimtoim Date: Tue, 21 Feb 2023 11:16:57 +0200 Subject: [PATCH 1/3] Fix flaky tests --- middleware/context_timeout_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/middleware/context_timeout_test.go b/middleware/context_timeout_test.go index 605ca8e65..404a83741 100644 --- a/middleware/context_timeout_test.go +++ b/middleware/context_timeout_test.go @@ -148,7 +148,7 @@ func TestContextTimeoutWithDefaultErrorMessage(t *testing.T) { c := e.NewContext(req, rec) err := m(func(c echo.Context) error { - if err := sleepWithContext(c.Request().Context(), time.Duration(20*time.Millisecond)); err != nil { + if err := sleepWithContext(c.Request().Context(), time.Duration(80*time.Millisecond)); err != nil { return err } return c.String(http.StatusOK, "Hello, World!") @@ -176,7 +176,7 @@ func TestContextTimeoutCanHandleContextDeadlineOnNextHandler(t *testing.T) { return nil } - timeout := 10 * time.Millisecond + timeout := 50 * time.Millisecond m := ContextTimeoutWithConfig(ContextTimeoutConfig{ Timeout: timeout, ErrorHandler: timeoutErrorHandler, @@ -189,11 +189,11 @@ func TestContextTimeoutCanHandleContextDeadlineOnNextHandler(t *testing.T) { c := e.NewContext(req, rec) err := m(func(c echo.Context) error { - // NOTE: when difference between timeout duration and handler execution time is almost the same (in range of 100microseconds) - // the result of timeout does not seem to be reliable - could respond timeout, could respond handler output - // difference over 500microseconds (0.5millisecond) response seems to be reliable + // extremely short periods are not reliable for tests when it comes to goroutines. We can not guarantee in which + // order scheduler decides do execute: 1) request goroutine, 2) timeout timer goroutine. + // most of the time we get result we expect but Mac OS seems to be quite flaky - if err := sleepWithContext(c.Request().Context(), time.Duration(20*time.Millisecond)); err != nil { + if err := sleepWithContext(c.Request().Context(), 100*time.Millisecond); err != nil { return err } From 01828cd1bb89a28c78b8a9efad95eb873fe923e6 Mon Sep 17 00:00:00 2001 From: toimtoimtoim Date: Tue, 21 Feb 2023 11:17:53 +0200 Subject: [PATCH 2/3] Fix tests failing on Go 1.20 on Windows. Clean works differently on 1.20. Use path.Clean instead with some workaround related to errors. --- middleware/static.go | 28 +++++++++++----------------- middleware/static_other.go | 12 ++++++++++++ middleware/static_windows.go | 23 +++++++++++++++++++++++ 3 files changed, 46 insertions(+), 17 deletions(-) create mode 100644 middleware/static_other.go create mode 100644 middleware/static_windows.go diff --git a/middleware/static.go b/middleware/static.go index 27ccf4117..24a5f59b9 100644 --- a/middleware/static.go +++ b/middleware/static.go @@ -8,7 +8,6 @@ import ( "net/url" "os" "path" - "path/filepath" "strings" "github.com/labstack/echo/v4" @@ -157,9 +156,9 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc { } // Index template - t, err := template.New("index").Parse(html) - if err != nil { - panic(fmt.Sprintf("echo: %v", err)) + t, tErr := template.New("index").Parse(html) + if tErr != nil { + panic(fmt.Errorf("echo: %w", tErr)) } return func(next echo.HandlerFunc) echo.HandlerFunc { @@ -176,7 +175,7 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc { if err != nil { return } - name := filepath.Join(config.Root, filepath.Clean("/"+p)) // "/"+ for security + name := path.Join(config.Root, path.Clean("/"+p)) // "/"+ for security if config.IgnoreBase { routePath := path.Base(strings.TrimRight(c.Path(), "/*")) @@ -187,12 +186,14 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc { } } - file, err := openFile(config.Filesystem, name) + file, err := config.Filesystem.Open(name) if err != nil { - if !os.IsNotExist(err) { + if !isIgnorableOpenFileError(err) { return err } + // file with that path did not exist, so we continue down in middleware/handler chain, hoping that we end up in + // handler that is meant to handle this request if err = next(c); err == nil { return err } @@ -202,7 +203,7 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc { return err } - file, err = openFile(config.Filesystem, filepath.Join(config.Root, config.Index)) + file, err = config.Filesystem.Open(path.Join(config.Root, config.Index)) if err != nil { return err } @@ -216,15 +217,13 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc { } if info.IsDir() { - index, err := openFile(config.Filesystem, filepath.Join(name, config.Index)) + index, err := config.Filesystem.Open(path.Join(name, config.Index)) if err != nil { if config.Browse { return listDir(t, name, file, c.Response()) } - if os.IsNotExist(err) { - return next(c) - } + return next(c) } defer index.Close() @@ -242,11 +241,6 @@ func StaticWithConfig(config StaticConfig) echo.MiddlewareFunc { } } -func openFile(fs http.FileSystem, name string) (http.File, error) { - pathWithSlashes := filepath.ToSlash(name) - return fs.Open(pathWithSlashes) -} - func serveFile(c echo.Context, file http.File, info os.FileInfo) error { http.ServeContent(c.Response(), c.Request(), info.Name(), info.ModTime(), file) return nil diff --git a/middleware/static_other.go b/middleware/static_other.go new file mode 100644 index 000000000..0337b22af --- /dev/null +++ b/middleware/static_other.go @@ -0,0 +1,12 @@ +//go:build !windows + +package middleware + +import ( + "os" +) + +// We ignore these errors as there could be handler that matches request path. +func isIgnorableOpenFileError(err error) bool { + return os.IsNotExist(err) +} diff --git a/middleware/static_windows.go b/middleware/static_windows.go new file mode 100644 index 000000000..a0164ff95 --- /dev/null +++ b/middleware/static_windows.go @@ -0,0 +1,23 @@ +package middleware + +import ( + "os" +) + +// We ignore these errors as there could be handler that matches request path. +// +// As of 1.20 on Windows filepath.Clean has different behaviour on OS related filesystems so we need to use path.Clean +// which is more suitable for path coming from web but this has some caveats on Windows. When we eventually end up in +// os related filesystem Open methods we are getting different errors as earlier versions. As of 1.20 path checks are +// more strict on path you provide and consider path with [UNC](https://en.wikipedia.org/wiki/Path_(computing)#UNC) +// but missing host etc parts as invalid. Previously it would result you `fs.ErrNotExist`. +// +// So for 1.20@Windows we need to consider it as same not exist so we can continue next middleware/handler and not error +// which would result status 500 instead of potential route hit or 404. +func isIgnorableOpenFileError(err error) bool { + if os.IsNotExist(err) { + return true + } + errTxt := err.Error() + return errTxt == "http: invalid or unsafe file path" || errTxt == "invalid path" +} From 558bf90073279ed865f052ab472f50e32c41bd37 Mon Sep 17 00:00:00 2001 From: toimtoimtoim Date: Tue, 21 Feb 2023 23:39:58 +0200 Subject: [PATCH 3/3] improve comments --- middleware/context_timeout_test.go | 5 ++--- middleware/static_windows.go | 14 +++++++------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/middleware/context_timeout_test.go b/middleware/context_timeout_test.go index 404a83741..24c6203e7 100644 --- a/middleware/context_timeout_test.go +++ b/middleware/context_timeout_test.go @@ -189,9 +189,8 @@ func TestContextTimeoutCanHandleContextDeadlineOnNextHandler(t *testing.T) { c := e.NewContext(req, rec) err := m(func(c echo.Context) error { - // extremely short periods are not reliable for tests when it comes to goroutines. We can not guarantee in which - // order scheduler decides do execute: 1) request goroutine, 2) timeout timer goroutine. - // most of the time we get result we expect but Mac OS seems to be quite flaky + // NOTE: Very short periods are not reliable for tests due to Go routine scheduling and the unpredictable order + // for 1) request and 2) time goroutine. For most OS this works as expected, but MacOS seems most flaky. if err := sleepWithContext(c.Request().Context(), 100*time.Millisecond); err != nil { return err diff --git a/middleware/static_windows.go b/middleware/static_windows.go index a0164ff95..0ab119859 100644 --- a/middleware/static_windows.go +++ b/middleware/static_windows.go @@ -6,14 +6,14 @@ import ( // We ignore these errors as there could be handler that matches request path. // -// As of 1.20 on Windows filepath.Clean has different behaviour on OS related filesystems so we need to use path.Clean -// which is more suitable for path coming from web but this has some caveats on Windows. When we eventually end up in -// os related filesystem Open methods we are getting different errors as earlier versions. As of 1.20 path checks are -// more strict on path you provide and consider path with [UNC](https://en.wikipedia.org/wiki/Path_(computing)#UNC) -// but missing host etc parts as invalid. Previously it would result you `fs.ErrNotExist`. +// As of Go 1.20 filepath.Clean has different behaviour on OS related filesystems so we need to use path.Clean +// on Windows which has some caveats. The Open methods might return different errors than earlier versions and +// as of 1.20 path checks are more strict on the provided path and considers [UNC](https://en.wikipedia.org/wiki/Path_(computing)#UNC) +// paths with missing host etc parts as invalid. Previously it would result you `fs.ErrNotExist`. // -// So for 1.20@Windows we need to consider it as same not exist so we can continue next middleware/handler and not error -// which would result status 500 instead of potential route hit or 404. +// For 1.20@Windows we need to treat those errors the same as `fs.ErrNotExists` so we can continue handling +// errors in the middleware/handler chain. Otherwise we might end up with status 500 instead of finding a route +// or return 404 not found. func isIgnorableOpenFileError(err error) bool { if os.IsNotExist(err) { return true