Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

📝 [Proposal]: Always return immutable string and deprecate the Immutable flag #3367

Open
3 tasks done
ksw2000 opened this issue Mar 22, 2025 · 9 comments
Open
3 tasks done

Comments

@ksw2000
Copy link
Member

ksw2000 commented Mar 22, 2025

Feature Proposal Description

In Golang, using mutable strings is not intuitive. Some code in Fiber modifies strings. In the current implementation, we use the Immutable flag to control whether the strings returned by certain functions are immutable.

Consider the following example:

func main() {
	o := newObj()
	s := o.GetB()
	fmt.Println(s)
	o.foo()
	fmt.Println(s)
}

type obj struct {
	b []byte
}

func newObj() obj {
	return obj{
		b: []byte("hey!"),
	}
}

func (o *obj) GetB() string {
	return utils.UnsafeString(o.b)
}

func (o *obj) foo() {
	for i := 0; i < len(o.b)/2; i++ {
		o.b[i], o.b[len(o.b)-i-1] = o.b[len(o.b)-i-1], o.b[i]
	}
}
> go run main.go
hey!
!yeh

In the above example code, the returned string s may be changed after calling foo. This is not intuitive in Golang!

The overhead of utils.UnsafeString is lower than directly copying a string. However, this approach can cause unnecessary issues.

In the Golang standard library, returned strings are always kept immutable. When performance is a concern, a mutable byte slice is returned instead. For example, in the bufio package, scanner.Bytes() returns a mutable byte slice, while scanner.Text() returns an immutable string:

https://cs.opensource.google/go/go/+/refs/tags/go1.24.1:src/bufio/scan.go;l=105-116;drc=bc2124dab14fa292e18df2937037d782f7868635

// Bytes returns the most recent token generated by a call to [Scanner.Scan].
// The underlying array may point to data that will be overwritten
// by a subsequent call to Scan. It does no allocation.
func (s *Scanner) Bytes() []byte {
	return s.token
}

// Text returns the most recent token generated by a call to [Scanner.Scan]
// as a newly allocated string holding its bytes.
func (s *Scanner) Text() string {
	return string(s.token)
}

Similarly, Fasthttp follows the same principle. Since we wrap Fasthttp, we must adhere to this rule as well. If users are concerned about performance, they can call fooBytes. Otherwise, they can use foo, which returns an immutable string.

Additionally, I believe the Immutable flag in Config is poorly designed. Users may not fully understand its scope or which functions it affects. Moreover, if a user wants to call function A, which returns an immutable string, while also calling function B to optimize performance without copying, this design does not allow it.

We should follow Golang's standard and Fasthttp’s principles: treating all strings as immutable while providing an alternative function for performance-sensitive cases.

For example, in ctx.go:

// OriginalURL contains the original request URL.
// Returned value is only valid within the handler. Do not store any references.
// Make copies or use the Immutable setting to use the value outside the handler.
func (c *DefaultCtx) OriginalURL() string {
	return c.app.getString(c.fasthttp.Request.Header.RequestURI())
}

We can refactor it as follows:

// OriginalURL contains the original request URL.
func (c *DefaultCtx) OriginalURL() string {
	return string(c.fasthttp.Request.Header.RequestURI())
}

// OriginalURLBytes returns the original request URL as a byte slice.
// The returned value is only valid within the handler. Do not store any references.
func (c *DefaultCtx) OriginalURLBytes() []byte {
	return c.fasthttp.Request.Header.RequestURI()
}

Related issue: #3236 #3246

Here are some parts that can be refactored

// BodyRaw contains the raw body submitted in a POST request.
// Returned value is only valid within the handler. Do not store any references.
// Make copies or use the Immutable setting instead.
func (c *DefaultCtx) BodyRaw() []byte {
	return c.getBody()
}

// CHANGE TO

func (c *DefaultCtx) BodyRaw() string {}
func (c *DefaultCtx) BodyRawBytes() []byte {}



// Cookies are used for getting a cookie value by key.
// Defaults to the empty string "" if the cookie doesn't exist.
// If a default value is given, it will return that value if the cookie doesn't exist.
// The returned value is only valid within the handler. Do not store any references.
// Make copies or use the Immutable setting to use the value outside the Handler.
func (c *DefaultCtx) Cookies(key string, defaultValue ...string) string {
	return defaultString(c.app.getString(c.fasthttp.Request.Header.Cookie(key)), defaultValue)
}

// CHANGED TO
func (c *DefaultCtx) Cookies(key string, defaultValue ...string) string {}
func (c *DefaultCtx) CookiesBytes(key string, defaultValue ...string) []byte {}



// FormValue returns the first value by key from a MultipartForm.
// Search is performed in QueryArgs, PostArgs, MultipartForm and FormFile in this particular order.
// Defaults to the empty string "" if the form value doesn't exist.
// If a default value is given, it will return that value if the form value does not exist.
// Returned value is only valid within the handler. Do not store any references.
// Make copies or use the Immutable setting instead.
func (c *DefaultCtx) FormValue(key string, defaultValue ...string) string {
	return defaultString(c.app.getString(c.fasthttp.FormValue(key)), defaultValue)
}

// CHANGE TO
func (c *DefaultCtx) FormValue(key string, defaultValue ...string) string {}
func (c *DefaultCtx) FormValueBytes(key string, defaultValue ...string) []byte {}



func (c *DefaultCtx) Path(override ...string) string {
    // ...
    return c.app.getString(c.path)
}

// CHANGE TO
func (c *DefaultCtx) Path(override ...string) string  {}
func (c *DefaultCtx) PathBytes(override ...string) string []byte {}

There are still a lot of functions can be refactored. I believe the most problematic aspect of Fiber is its use of mutable string, which can easily be misused even in small projects. Additionally, maintaining the Immutable flag requires a significant amount of maintenance effort. I think this issue needs to be addressed in the Fiber v3.

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have searched for existing issues that describe my proposal before opening this one.
  • I understand that a proposal that does not meet these guidelines may be closed without explanation.
@ksw2000 ksw2000 changed the title 📝 [Proposal]: Always return immutable string and dispose of the Immutable flag! 📝 [Proposal]: Always return immutable string and deprecate the Immutable flag Mar 22, 2025
@gaby
Copy link
Member

gaby commented Mar 22, 2025

@ksw2000 Kind of agree with you. The thing i'm not a fan of is having to add so many new functions to be able to return "bytes"

@ksw2000
Copy link
Member Author

ksw2000 commented Mar 22, 2025

@gaby Do you have any ideas? I think the first step is to try removing the Immutable flag and refactoring the code. I can do this work and estimate it will take a week.

@ReneWerner87
Copy link
Member

@Fenny What do you think would be against the design concept of being close to the api of expressjs? Also, the promise of zero allocations would no longer apply to string methods?

@gaby
Copy link
Member

gaby commented Mar 26, 2025

I added a new set of benchmarks with Immutable enabled. For the basic case of a simple route that just returns the performance is 9ns/op slower. When benchmarking in parallel the performance is 2-3ns/op slower, but looking at the iterations cound it's around 400k slower.

ubuntu@ubuntu:~/Desktop/git/fiber$ go test -v ./... -run=^$ -bench=Benchmark_Router_Next_Default -benchmem -count=4
goos: linux
goarch: amd64
pkg: github.com/gofiber/fiber/v3
cpu: AMD Ryzen 7 7800X3D 8-Core Processor           
Benchmark_Router_Next_Default
Benchmark_Router_Next_Default-4                      	35451170	        31.42 ns/op	       0 B/op	       0 allocs/op
Benchmark_Router_Next_Default-4                      	37581970	        31.55 ns/op	       0 B/op	       0 allocs/op
Benchmark_Router_Next_Default-4                      	37955616	        31.86 ns/op	       0 B/op	       0 allocs/op
Benchmark_Router_Next_Default-4                      	37459659	        31.51 ns/op	       0 B/op	       0 allocs/op
Benchmark_Router_Next_Default_Parallel
Benchmark_Router_Next_Default_Parallel-4             	147950630	         8.087 ns/op	       0 B/op	       0 allocs/op
Benchmark_Router_Next_Default_Parallel-4             	141775549	         8.234 ns/op	       0 B/op	       0 allocs/op
Benchmark_Router_Next_Default_Parallel-4             	145090087	         9.321 ns/op	       0 B/op	       0 allocs/op
Benchmark_Router_Next_Default_Parallel-4             	100000000	        10.03 ns/op	       0 B/op	       0 allocs/op
Benchmark_Router_Next_Default_Immutable
Benchmark_Router_Next_Default_Immutable-4            	26837396	        40.83 ns/op	       3 B/op	       1 allocs/op
Benchmark_Router_Next_Default_Immutable-4            	28757527	        40.87 ns/op	       3 B/op	       1 allocs/op
Benchmark_Router_Next_Default_Immutable-4            	27952845	        41.01 ns/op	       3 B/op	       1 allocs/op
Benchmark_Router_Next_Default_Immutable-4            	28038631	        41.55 ns/op	       3 B/op	       1 allocs/op
Benchmark_Router_Next_Default_Parallel_Immutable
Benchmark_Router_Next_Default_Parallel_Immutable-4   	100000000	        12.11 ns/op	       3 B/op	       1 allocs/op
Benchmark_Router_Next_Default_Parallel_Immutable-4   	106113111	        11.52 ns/op	       3 B/op	       1 allocs/op
Benchmark_Router_Next_Default_Parallel_Immutable-4   	100000000	        11.26 ns/op	       3 B/op	       1 allocs/op
Benchmark_Router_Next_Default_Parallel_Immutable-4   	100000000	        11.32 ns/op	       3 B/op	       1 allocs/op

@ReneWerner87
Copy link
Member

ReneWerner87 commented Mar 26, 2025

for version 3 we want to stay like it is for now, maybe with the next version we can do it
-> we have voted internally in the maintainer circle

we have to consider this further first, because then the api's are not really the same as the express api's
or we have many additional methods with byte suffix

keep the idea open

@ksw2000
Copy link
Member Author

ksw2000 commented Mar 26, 2025

for version 3 we want to stay like it is for now, maybe with the next version we can do it -> we have voted internally in the maintainer circle

we have to consider this further first, because then the api's are not really the same as the express api's or we have many additional methods with byte suffix

keep the idea open

What if we use immutable strings always instead of adding new methods with byte suffixes?

@ReneWerner87
Copy link
Member

ReneWerner87 commented Mar 26, 2025

then the performance of the fiber framework would fall sharply and we would violate the basic concept that we are trying to achieve zero allocations

@ReneWerner87
Copy link
Member

Image
that is our promise and we should stick to it, which is why so many users love fiber

express like and zero allocation and therefore very good performance

@ksw2000
Copy link
Member Author

ksw2000 commented Mar 26, 2025

I have an idea, we can create our String type wrapping []byte.

type String []byte

// copy the string
func (s String) String() string {
	return string(s)
}

// zero-allocation string
func (s String) MutableString() string {
	return utils.UnsafeString(s)
}

// zero-allocation
func (s String) Bytes() []byte {
	return s
}

I think this approach does not require adding extra functions, e.g. Foo(), FooBytes(). Instead, users can choose to use zero-copy, deep copy, or directly return a byte slice.

func (c *DefaultCtx) Path(override ...string) String {
	if len(override) != 0 && string(c.path) != override[0] {
		// Set new path to context
		c.pathOriginal = override[0]

		// Set new path to request context
		c.fasthttp.Request.URI().SetPath(c.pathOriginal)
		// Prettify path
		c.configDependentPaths()
	}
        // we can directly return c.path, the type of c.path is []byte
	// return c.app.getString(c.path) <-- Let user decide to use []byte, string, or unsafe string
	return c.path
}

Additionally, testing indicates that calling this function does not seem to introduce any extra overhead.

func Benchmark_App_Path_Pure(b *testing.B) {
	app := New()
	c := app.AcquireCtx(&fasthttp.RequestCtx{})
	c.Path("/foo/bar")
	for i := 0; i < b.N; i++ {
		_ = c.Path()
	}
}

func Benchmark_App_Path_Bytes(b *testing.B) {
	app := New()
	c := app.AcquireCtx(&fasthttp.RequestCtx{})
	c.Path("/foo/bar")
	for i := 0; i < b.N; i++ {
		_ = c.Path().Bytes()
	}
}

func Benchmark_App_Path_MutableString(b *testing.B) {
	app := New()
	c := app.AcquireCtx(&fasthttp.RequestCtx{})
	c.Path("/foo/bar")
	for i := 0; i < b.N; i++ {
		_ = c.Path().MutableString()
	}
}

func Benchmark_App_Path_String(b *testing.B) {
	app := New()
	c := app.AcquireCtx(&fasthttp.RequestCtx{})
	c.Path("/foo/bar")
	for i := 0; i < b.N; i++ {
		_ = c.Path().String()
	}
}
goos: windows
goarch: amd64
pkg: github.com/gofiber/fiber/v3
cpu: Intel(R) Core(TM) i5-8250U CPU @ 1.60GHz
Benchmark_App_Path_Pure-8               240556743                4.822 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_Pure-8               256751659                4.540 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_Pure-8               243714748                4.534 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_Pure-8               259317325                4.798 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_Bytes-8              261279774                4.861 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_Bytes-8              244614405                4.587 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_Bytes-8              250186806                4.479 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_Bytes-8              292455738                4.369 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_MutableString-8      261749498                4.421 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_MutableString-8      242989892                4.520 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_MutableString-8      262126117                4.648 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_MutableString-8      259941178                4.652 ns/op           0 B/op          0 allocs/op
Benchmark_App_Path_String-8             100000000               10.38 ns/op            0 B/op          0 allocs/op
Benchmark_App_Path_String-8             100000000               10.03 ns/op            0 B/op          0 allocs/op
Benchmark_App_Path_String-8             102465264               12.64 ns/op            0 B/op          0 allocs/op
Benchmark_App_Path_String-8             90210640                11.80 ns/op            0 B/op          0 allocs/op
PASS

The benchmark shows that wrapping the []byte does not seem to incur additional cost. The zero-allocation copy took less time than the deep copy, which was also expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants