Skip to content
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,14 +290,16 @@ func Test_App_BodyLimit_Negative(t *testing.T) {
return c.SendStatus(StatusOK)
})

cfg := TestConfig{Timeout: 0, FailOnTimeout: false}

largeBody := bytes.Repeat([]byte{'a'}, DefaultBodyLimit+1)
req := httptest.NewRequest(MethodPost, "/", bytes.NewReader(largeBody))
_, err := app.Test(req)
_, err := app.Test(req, cfg)
require.ErrorIs(t, err, fasthttp.ErrBodyTooLarge)

smallBody := bytes.Repeat([]byte{'a'}, DefaultBodyLimit-1)
req = httptest.NewRequest(MethodPost, "/", bytes.NewReader(smallBody))
resp, err := app.Test(req)
resp, err := app.Test(req, cfg)
require.NoError(t, err)
require.Equal(t, StatusOK, resp.StatusCode)
}
Expand All @@ -314,12 +316,14 @@ func Test_App_BodyLimit_Zero(t *testing.T) {

largeBody := bytes.Repeat([]byte{'a'}, DefaultBodyLimit+1)
req := httptest.NewRequest(MethodPost, "/", bytes.NewReader(largeBody))
_, err := app.Test(req)
timeoutCfg := TestConfig{Timeout: 5 * time.Second}

_, err := app.Test(req, timeoutCfg)
require.ErrorIs(t, err, fasthttp.ErrBodyTooLarge)

smallBody := bytes.Repeat([]byte{'a'}, DefaultBodyLimit-1)
req = httptest.NewRequest(MethodPost, "/", bytes.NewReader(smallBody))
resp, err := app.Test(req)
resp, err := app.Test(req, timeoutCfg)
require.NoError(t, err)
require.Equal(t, StatusOK, resp.StatusCode)
}
Expand All @@ -334,17 +338,19 @@ func Test_App_BodyLimit_LargerThanDefault(t *testing.T) {
return c.SendStatus(StatusOK)
})

timeoutCfg := TestConfig{Timeout: 5 * time.Second}

// Body larger than the default but within our custom limit should succeed
midBody := bytes.Repeat([]byte{'a'}, DefaultBodyLimit+512)
req := httptest.NewRequest(MethodPost, "/", bytes.NewReader(midBody))
resp, err := app.Test(req)
resp, err := app.Test(req, timeoutCfg)
require.NoError(t, err)
require.Equal(t, StatusOK, resp.StatusCode)

// Body above the custom limit should fail
largeBody := bytes.Repeat([]byte{'a'}, limit+1)
req = httptest.NewRequest(MethodPost, "/", bytes.NewReader(largeBody))
_, err = app.Test(req)
_, err = app.Test(req, timeoutCfg)
require.ErrorIs(t, err, fasthttp.ErrBodyTooLarge)
}

Expand Down
79 changes: 79 additions & 0 deletions binder/binder.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package binder

import (
"errors"
"mime/multipart"
"sync"
)

Expand Down Expand Up @@ -71,6 +72,28 @@ var MsgPackBinderPool = sync.Pool{
},
}

const (
stringSliceMapDefaultCap = 8
stringSliceMapMaxEntries = 128
)

var stringSliceMapPool = sync.Pool{
New: func() any {
return make(map[string][]string, stringSliceMapDefaultCap)
},
}

const (
fileHeaderSliceMapDefaultCap = 4
fileHeaderSliceMapMaxEntries = 64
)

var fileHeaderSliceMapPool = sync.Pool{
New: func() any {
return make(map[string][]*multipart.FileHeader, fileHeaderSliceMapDefaultCap)
},
}

// GetFromThePool retrieves a binder from the provided sync.Pool and panics if
// the stored value cannot be cast to the requested type.
func GetFromThePool[T any](pool *sync.Pool) T {
Expand All @@ -86,3 +109,59 @@ func GetFromThePool[T any](pool *sync.Pool) T {
func PutToThePool[T any](pool *sync.Pool, binder T) {
pool.Put(binder)
}

func acquireStringSliceMap() map[string][]string {
m, ok := stringSliceMapPool.Get().(map[string][]string)
if !ok {
panic(errors.New("failed to type-assert to map[string][]string"))
}
if m == nil {
return make(map[string][]string, stringSliceMapDefaultCap)
}
Comment on lines +118 to +120
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This if m == nil check appears to be redundant. The stringSliceMapPool is initialized with a New function that always returns a non-nil map[string][]string. Therefore, stringSliceMapPool.Get() will not return nil, and this code block is unreachable.

if len(m) > 0 {
clear(m)
}
return m
}

func releaseStringSliceMap(m map[string][]string) {
if m == nil {
return
}
used := len(m)
if used > 0 {
clear(m)
}
Comment on lines +132 to +134
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The map is cleared here before being put back into the pool, but it's also cleared in acquireStringSliceMap when retrieved from the pool. While this doesn't cause any harm, it's redundant. To improve clarity and have a single point of responsibility for resetting the object's state, it's conventional to clear/reset objects on acquisition from a pool.

if used > stringSliceMapMaxEntries {
return
}
stringSliceMapPool.Put(m)
}

func acquireFileHeaderSliceMap() map[string][]*multipart.FileHeader {
m, ok := fileHeaderSliceMapPool.Get().(map[string][]*multipart.FileHeader)
if !ok {
panic(errors.New("failed to type-assert to map[string][]*multipart.FileHeader"))
}
if m == nil {
return make(map[string][]*multipart.FileHeader, fileHeaderSliceMapDefaultCap)
}
Comment on lines +146 to +148
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to acquireStringSliceMap, this if m == nil check is redundant because fileHeaderSliceMapPool.New always returns a non-nil map, making this block unreachable.

if len(m) > 0 {
clear(m)
}
return m
}

func releaseFileHeaderSliceMap(m map[string][]*multipart.FileHeader) {
if m == nil {
return
}
used := len(m)
if used > 0 {
clear(m)
}
Comment on lines +160 to +162
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to releaseStringSliceMap, the map is cleared here before being put back into the pool, but it's also cleared in acquireFileHeaderSliceMap. This is redundant. It's better to only clear the map on acquisition.

if used > fileHeaderSliceMapMaxEntries {
return
}
fileHeaderSliceMapPool.Put(m)
}
3 changes: 2 additions & 1 deletion binder/cookie.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ func (*CookieBinding) Name() string {

// Bind parses the request cookie and returns the result.
func (b *CookieBinding) Bind(req *fasthttp.Request, out any) error {
data := make(map[string][]string)
data := acquireStringSliceMap()
defer releaseStringSliceMap(data)

for key, val := range req.Header.Cookies() {
k := utils.UnsafeString(key)
Expand Down
15 changes: 9 additions & 6 deletions binder/form.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package binder

import (
"mime/multipart"

utils "github.com/gofiber/utils/v2"
"github.com/valyala/fasthttp"
)
Expand All @@ -21,13 +19,14 @@ func (*FormBinding) Name() string {

// Bind parses the request body and returns the result.
func (b *FormBinding) Bind(req *fasthttp.Request, out any) error {
data := make(map[string][]string)

// Handle multipart form
if FilterFlags(utils.UnsafeString(req.Header.ContentType())) == MIMEMultipartForm {
return b.bindMultipart(req, out)
}

data := acquireStringSliceMap()
defer releaseStringSliceMap(data)

for key, val := range req.PostArgs().All() {
k := utils.UnsafeString(key)
v := utils.UnsafeString(val)
Expand All @@ -46,15 +45,19 @@ func (b *FormBinding) bindMultipart(req *fasthttp.Request, out any) error {
return err
}

data := make(map[string][]string)
data := acquireStringSliceMap()
defer releaseStringSliceMap(data)

files := acquireFileHeaderSliceMap()
defer releaseFileHeaderSliceMap(files)

for key, values := range multipartForm.Value {
err = formatBindData(b.Name(), out, data, key, values, b.EnableSplitting, true)
if err != nil {
return err
}
}

files := make(map[string][]*multipart.FileHeader)
for key, values := range multipartForm.File {
err = formatBindData(b.Name(), out, files, key, values, b.EnableSplitting, true)
if err != nil {
Expand Down
4 changes: 3 additions & 1 deletion binder/header.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ func (*HeaderBinding) Name() string {

// Bind parses the request header and returns the result.
func (b *HeaderBinding) Bind(req *fasthttp.Request, out any) error {
data := make(map[string][]string)
data := acquireStringSliceMap()
defer releaseStringSliceMap(data)

for key, val := range req.Header.All() {
k := utils.UnsafeString(key)
v := utils.UnsafeString(val)
Expand Down
4 changes: 3 additions & 1 deletion binder/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ func (*QueryBinding) Name() string {

// Bind parses the request query and returns the result.
func (b *QueryBinding) Bind(reqCtx *fasthttp.Request, out any) error {
data := make(map[string][]string)
data := acquireStringSliceMap()
defer releaseStringSliceMap(data)

for key, val := range reqCtx.URI().QueryArgs().All() {
k := utils.UnsafeString(key)
v := utils.UnsafeString(val)
Expand Down
3 changes: 2 additions & 1 deletion binder/resp_header.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ func (*RespHeaderBinding) Name() string {

// Bind parses the response header and returns the result.
func (b *RespHeaderBinding) Bind(resp *fasthttp.Response, out any) error {
data := make(map[string][]string)
data := acquireStringSliceMap()
defer releaseStringSliceMap(data)

for key, val := range resp.Header.All() {
k := utils.UnsafeString(key)
Expand Down
4 changes: 3 additions & 1 deletion binder/uri.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ func (*URIBinding) Name() string {

// Bind parses the URI parameters and returns the result.
func (b *URIBinding) Bind(params []string, paramsFunc func(key string, defaultValue ...string) string, out any) error {
data := make(map[string][]string, len(params))
data := acquireStringSliceMap()
defer releaseStringSliceMap(data)

for _, param := range params {
data[param] = append(data[param], paramsFunc(param))
}
Expand Down
Loading
Loading