From 7d147928ee232fce156ea7ce8ae6329e148aeb41 Mon Sep 17 00:00:00 2001 From: Kostadin Plachkov <20207730+kplachkov@users.noreply.github.com> Date: Wed, 8 May 2024 04:13:36 +0300 Subject: [PATCH 1/3] fix(gin): data race warning for gin mode (#1580) * fix: data race warning (#1180) * Fix the tests * refactor: remove unnecessary imports and optimize codebase - Remove unnecessary import of `flag` Signed-off-by: Bo-Yi Wu * test: refactor test assertions for mode settings - Update test assertions for mode setting in `mode_test.go` Signed-off-by: Bo-Yi Wu --------- Signed-off-by: Bo-Yi Wu Co-authored-by: Bo-Yi Wu --- debug.go | 3 ++- mode.go | 20 +++++++++----------- mode_test.go | 19 ++++++------------- 3 files changed, 17 insertions(+), 25 deletions(-) diff --git a/debug.go b/debug.go index 1761fe32..0d808f10 100644 --- a/debug.go +++ b/debug.go @@ -10,6 +10,7 @@ import ( "runtime" "strconv" "strings" + "sync/atomic" ) const ginSupportMinGoVer = 18 @@ -17,7 +18,7 @@ const ginSupportMinGoVer = 18 // IsDebugging returns true if the framework is running in debug mode. // Use SetMode(gin.ReleaseMode) to disable debug mode. func IsDebugging() bool { - return ginMode == debugCode + return atomic.LoadInt32(&ginMode) == debugCode } // DebugPrintRouteFunc indicates debug log output format. diff --git a/mode.go b/mode.go index fd26d907..13aa3be0 100644 --- a/mode.go +++ b/mode.go @@ -8,6 +8,7 @@ import ( "flag" "io" "os" + "sync/atomic" "github.com/gin-gonic/gin/binding" ) @@ -43,10 +44,8 @@ var DefaultWriter io.Writer = os.Stdout // DefaultErrorWriter is the default io.Writer used by Gin to debug errors var DefaultErrorWriter io.Writer = os.Stderr -var ( - ginMode = debugCode - modeName = DebugMode -) +var ginMode int32 = debugCode +var modeName atomic.Value func init() { mode := os.Getenv(EnvGinMode) @@ -64,17 +63,16 @@ func SetMode(value string) { } switch value { - case DebugMode: - ginMode = debugCode + case DebugMode, "": + atomic.StoreInt32(&ginMode, debugCode) case ReleaseMode: - ginMode = releaseCode + atomic.StoreInt32(&ginMode, releaseCode) case TestMode: - ginMode = testCode + atomic.StoreInt32(&ginMode, testCode) default: panic("gin mode unknown: " + value + " (available mode: debug release test)") } - - modeName = value + modeName.Store(value) } // DisableBindValidation closes the default validator. @@ -96,5 +94,5 @@ func EnableJsonDecoderDisallowUnknownFields() { // Mode returns current gin mode. func Mode() string { - return modeName + return modeName.Load().(string) } diff --git a/mode_test.go b/mode_test.go index 2407f463..be03a9d0 100644 --- a/mode_test.go +++ b/mode_test.go @@ -5,8 +5,8 @@ package gin import ( - "flag" "os" + "sync/atomic" "testing" "github.com/gin-gonic/gin/binding" @@ -18,31 +18,24 @@ func init() { } func TestSetMode(t *testing.T) { - assert.Equal(t, testCode, ginMode) + assert.Equal(t, int32(testCode), atomic.LoadInt32(&ginMode)) assert.Equal(t, TestMode, Mode()) os.Unsetenv(EnvGinMode) SetMode("") - assert.Equal(t, testCode, ginMode) + assert.Equal(t, int32(testCode), atomic.LoadInt32(&ginMode)) assert.Equal(t, TestMode, Mode()) - tmp := flag.CommandLine - flag.CommandLine = flag.NewFlagSet("", flag.ContinueOnError) - SetMode("") - assert.Equal(t, debugCode, ginMode) - assert.Equal(t, DebugMode, Mode()) - flag.CommandLine = tmp - SetMode(DebugMode) - assert.Equal(t, debugCode, ginMode) + assert.Equal(t, int32(debugCode), atomic.LoadInt32(&ginMode)) assert.Equal(t, DebugMode, Mode()) SetMode(ReleaseMode) - assert.Equal(t, releaseCode, ginMode) + assert.Equal(t, int32(releaseCode), atomic.LoadInt32(&ginMode)) assert.Equal(t, ReleaseMode, Mode()) SetMode(TestMode) - assert.Equal(t, testCode, ginMode) + assert.Equal(t, int32(testCode), atomic.LoadInt32(&ginMode)) assert.Equal(t, TestMode, Mode()) assert.Panics(t, func() { SetMode("unknown") }) From b1c1e7b572f76071fb0e0e7884a0697e0458aa7c Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Wed, 8 May 2024 10:14:42 +0800 Subject: [PATCH 2/3] ci: update Go version requirements and remove test files (#3957) - Update the Go version requirements in `.github/workflows/gin.yml` - Remove test files for Go versions 1.18 and 1.19 - Update the required Go version in `debug.go` and `debug_test.go` - Rename and modify files related to Go version 1.19 and 1.20 in the `internal/bytesconv` directory Signed-off-by: Bo-Yi Wu --- .github/workflows/gin.yml | 2 +- context_1.18_test.go | 37 ------------------- context_1.19_test.go | 30 --------------- context_test.go | 13 +++++++ debug.go | 2 +- debug_test.go | 2 +- .../{bytesconv_1.20.go => bytesconv.go} | 2 - internal/bytesconv/bytesconv_1.19.go | 26 ------------- 8 files changed, 16 insertions(+), 98 deletions(-) delete mode 100644 context_1.18_test.go delete mode 100644 context_1.19_test.go rename internal/bytesconv/{bytesconv_1.20.go => bytesconv.go} (97%) delete mode 100644 internal/bytesconv/bytesconv_1.19.go diff --git a/.github/workflows/gin.yml b/.github/workflows/gin.yml index 2e434341..4a9dbc93 100644 --- a/.github/workflows/gin.yml +++ b/.github/workflows/gin.yml @@ -34,7 +34,7 @@ jobs: strategy: matrix: os: [ubuntu-latest, macos-latest] - go: ["1.18", "1.19", "1.20", "1.21", "1.22"] + go: ["1.20", "1.21", "1.22"] test-tags: ["", "-tags nomsgpack", '-tags "sonic avx"', "-tags go_json", "-race"] include: diff --git a/context_1.18_test.go b/context_1.18_test.go deleted file mode 100644 index 6118beaa..00000000 --- a/context_1.18_test.go +++ /dev/null @@ -1,37 +0,0 @@ -// Copyright 2021 Gin Core Team. All rights reserved. -// Use of this source code is governed by a MIT style -// license that can be found in the LICENSE file. - -//go:build !go1.19 - -package gin - -import ( - "bytes" - "mime/multipart" - "net/http" - "net/http/httptest" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestContextFormFileFailed18(t *testing.T) { - buf := new(bytes.Buffer) - mw := multipart.NewWriter(buf) - defer func(mw *multipart.Writer) { - err := mw.Close() - if err != nil { - assert.Error(t, err) - } - }(mw) - c, _ := CreateTestContext(httptest.NewRecorder()) - c.Request, _ = http.NewRequest("POST", "/", nil) - c.Request.Header.Set("Content-Type", mw.FormDataContentType()) - c.engine.MaxMultipartMemory = 8 << 20 - assert.Panics(t, func() { - f, err := c.FormFile("file") - assert.Error(t, err) - assert.Nil(t, f) - }) -} diff --git a/context_1.19_test.go b/context_1.19_test.go deleted file mode 100644 index dd75325b..00000000 --- a/context_1.19_test.go +++ /dev/null @@ -1,30 +0,0 @@ -// Copyright 2022 Gin Core Team. All rights reserved. -// Use of this source code is governed by a MIT style -// license that can be found in the LICENSE file. - -//go:build go1.19 - -package gin - -import ( - "bytes" - "mime/multipart" - "net/http" - "net/http/httptest" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestContextFormFileFailed19(t *testing.T) { - buf := new(bytes.Buffer) - mw := multipart.NewWriter(buf) - mw.Close() - c, _ := CreateTestContext(httptest.NewRecorder()) - c.Request, _ = http.NewRequest("POST", "/", nil) - c.Request.Header.Set("Content-Type", mw.FormDataContentType()) - c.engine.MaxMultipartMemory = 8 << 20 - f, err := c.FormFile("file") - assert.Error(t, err) - assert.Nil(t, f) -} diff --git a/context_test.go b/context_test.go index e9bbae52..ae34c659 100644 --- a/context_test.go +++ b/context_test.go @@ -90,6 +90,19 @@ func TestContextFormFile(t *testing.T) { assert.NoError(t, c.SaveUploadedFile(f, "test")) } +func TestContextFormFileFailed(t *testing.T) { + buf := new(bytes.Buffer) + mw := multipart.NewWriter(buf) + mw.Close() + c, _ := CreateTestContext(httptest.NewRecorder()) + c.Request, _ = http.NewRequest("POST", "/", nil) + c.Request.Header.Set("Content-Type", mw.FormDataContentType()) + c.engine.MaxMultipartMemory = 8 << 20 + f, err := c.FormFile("file") + assert.Error(t, err) + assert.Nil(t, f) +} + func TestContextMultipartForm(t *testing.T) { buf := new(bytes.Buffer) mw := multipart.NewWriter(buf) diff --git a/debug.go b/debug.go index 0d808f10..ae346e9c 100644 --- a/debug.go +++ b/debug.go @@ -78,7 +78,7 @@ func getMinVer(v string) (uint64, error) { func debugPrintWARNINGDefault() { if v, e := getMinVer(runtime.Version()); e == nil && v < ginSupportMinGoVer { - debugPrint(`[WARNING] Now Gin requires Go 1.18+. + debugPrint(`[WARNING] Now Gin requires Go 1.20+. `) } diff --git a/debug_test.go b/debug_test.go index 2d5e9a56..e3909729 100644 --- a/debug_test.go +++ b/debug_test.go @@ -104,7 +104,7 @@ func TestDebugPrintWARNINGDefault(t *testing.T) { }) m, e := getMinVer(runtime.Version()) if e == nil && m < ginSupportMinGoVer { - assert.Equal(t, "[GIN-debug] [WARNING] Now Gin requires Go 1.18+.\n\n[GIN-debug] [WARNING] Creating an Engine instance with the Logger and Recovery middleware already attached.\n\n", re) + assert.Equal(t, "[GIN-debug] [WARNING] Now Gin requires Go 1.20+.\n\n[GIN-debug] [WARNING] Creating an Engine instance with the Logger and Recovery middleware already attached.\n\n", re) } else { assert.Equal(t, "[GIN-debug] [WARNING] Creating an Engine instance with the Logger and Recovery middleware already attached.\n\n", re) } diff --git a/internal/bytesconv/bytesconv_1.20.go b/internal/bytesconv/bytesconv.go similarity index 97% rename from internal/bytesconv/bytesconv_1.20.go rename to internal/bytesconv/bytesconv.go index 5b6040a6..a02c53c3 100644 --- a/internal/bytesconv/bytesconv_1.20.go +++ b/internal/bytesconv/bytesconv.go @@ -2,8 +2,6 @@ // Use of this source code is governed by a MIT style // license that can be found in the LICENSE file. -//go:build go1.20 - package bytesconv import ( diff --git a/internal/bytesconv/bytesconv_1.19.go b/internal/bytesconv/bytesconv_1.19.go deleted file mode 100644 index 669c9c91..00000000 --- a/internal/bytesconv/bytesconv_1.19.go +++ /dev/null @@ -1,26 +0,0 @@ -// Copyright 2020 Gin Core Team. All rights reserved. -// Use of this source code is governed by a MIT style -// license that can be found in the LICENSE file. - -//go:build !go1.20 - -package bytesconv - -import ( - "unsafe" -) - -// StringToBytes converts string to byte slice without a memory allocation. -func StringToBytes(s string) []byte { - return *(*[]byte)(unsafe.Pointer( - &struct { - string - Cap int - }{s, len(s)}, - )) -} - -// BytesToString converts byte slice to string without a memory allocation. -func BytesToString(b []byte) string { - return *(*string)(unsafe.Pointer(&b)) -} From 8791c96960e719ff2f41e24163c5898656cee474 Mon Sep 17 00:00:00 2001 From: Johannes Eiglsperger Date: Wed, 8 May 2024 09:47:54 +0200 Subject: [PATCH 3/3] feat(fs): Export, test and document OnlyFilesFS (#3939) --- fs.go | 52 ++++++++++++++++++++---------------- fs_test.go | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++ routergroup.go | 2 +- 3 files changed, 101 insertions(+), 24 deletions(-) create mode 100644 fs_test.go diff --git a/fs.go b/fs.go index f17d7434..51c3db86 100644 --- a/fs.go +++ b/fs.go @@ -9,37 +9,43 @@ import ( "os" ) -type onlyFilesFS struct { - fs http.FileSystem +// OnlyFilesFS implements an http.FileSystem without `Readdir` functionality. +type OnlyFilesFS struct { + FileSystem http.FileSystem } -type neuteredReaddirFile struct { - http.File -} +// Open passes `Open` to the upstream implementation without `Readdir` functionality. +func (o OnlyFilesFS) Open(name string) (http.File, error) { + f, err := o.FileSystem.Open(name) -// Dir returns a http.FileSystem that can be used by http.FileServer(). It is used internally -// in router.Static(). -// if listDirectory == true, then it works the same as http.Dir() otherwise it returns -// a filesystem that prevents http.FileServer() to list the directory files. -func Dir(root string, listDirectory bool) http.FileSystem { - fs := http.Dir(root) - if listDirectory { - return fs - } - return &onlyFilesFS{fs} -} - -// Open conforms to http.Filesystem. -func (fs onlyFilesFS) Open(name string) (http.File, error) { - f, err := fs.fs.Open(name) if err != nil { return nil, err } - return neuteredReaddirFile{f}, nil + + return neutralizedReaddirFile{f}, nil } -// Readdir overrides the http.File default implementation. -func (f neuteredReaddirFile) Readdir(_ int) ([]os.FileInfo, error) { +// neutralizedReaddirFile wraps http.File with a specific implementation of `Readdir`. +type neutralizedReaddirFile struct { + http.File +} + +// Readdir overrides the http.File default implementation and always returns nil. +func (n neutralizedReaddirFile) Readdir(_ int) ([]os.FileInfo, error) { // this disables directory listing return nil, nil } + +// Dir returns an http.FileSystem that can be used by http.FileServer(). +// It is used internally in router.Static(). +// if listDirectory == true, then it works the same as http.Dir(), +// otherwise it returns a filesystem that prevents http.FileServer() to list the directory files. +func Dir(root string, listDirectory bool) http.FileSystem { + fs := http.Dir(root) + + if listDirectory { + return fs + } + + return &OnlyFilesFS{FileSystem: fs} +} diff --git a/fs_test.go b/fs_test.go new file mode 100644 index 00000000..a1690cd9 --- /dev/null +++ b/fs_test.go @@ -0,0 +1,71 @@ +package gin + +import ( + "errors" + "net/http" + "os" + "testing" + + "github.com/stretchr/testify/assert" +) + +type mockFileSystem struct { + open func(name string) (http.File, error) +} + +func (m *mockFileSystem) Open(name string) (http.File, error) { + return m.open(name) +} + +func TestOnlyFilesFS_Open(t *testing.T) { + var testFile *os.File + mockFS := &mockFileSystem{ + open: func(name string) (http.File, error) { + return testFile, nil + }, + } + fs := &OnlyFilesFS{FileSystem: mockFS} + + file, err := fs.Open("foo") + + assert.NoError(t, err) + assert.Equal(t, testFile, file.(neutralizedReaddirFile).File) +} + +func TestOnlyFilesFS_Open_err(t *testing.T) { + testError := errors.New("mock") + mockFS := &mockFileSystem{ + open: func(_ string) (http.File, error) { + return nil, testError + }, + } + fs := &OnlyFilesFS{FileSystem: mockFS} + + file, err := fs.Open("foo") + + assert.ErrorIs(t, err, testError) + assert.Nil(t, file) +} + +func Test_neuteredReaddirFile_Readdir(t *testing.T) { + n := neutralizedReaddirFile{} + + res, err := n.Readdir(0) + + assert.NoError(t, err) + assert.Nil(t, res) +} + +func TestDir_listDirectory(t *testing.T) { + testRoot := "foo" + fs := Dir(testRoot, true) + + assert.Equal(t, http.Dir(testRoot), fs) +} + +func TestDir(t *testing.T) { + testRoot := "foo" + fs := Dir(testRoot, false) + + assert.Equal(t, &OnlyFilesFS{FileSystem: http.Dir(testRoot)}, fs) +} diff --git a/routergroup.go b/routergroup.go index c833fe8f..b2540ec1 100644 --- a/routergroup.go +++ b/routergroup.go @@ -218,7 +218,7 @@ func (group *RouterGroup) createStaticHandler(relativePath string, fs http.FileS fileServer := http.StripPrefix(absolutePath, http.FileServer(fs)) return func(c *Context) { - if _, noListing := fs.(*onlyFilesFS); noListing { + if _, noListing := fs.(*OnlyFilesFS); noListing { c.Writer.WriteHeader(http.StatusNotFound) }