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

🚀 Feature: "CopyBytes" and "CopyString" should only copy if "config.Immutable" is false #2285

Open
3 tasks done
leonklingele opened this issue Jan 6, 2023 · 5 comments

Comments

@leonklingele
Copy link
Member

Feature Description

Currently, CopyBytes and CopyString make a copy of the underlying data even if not required, i.e., when config.Immutable is set.
In order to improve performance, there should be a app.CopyBytes or ctx.CopyBytes which only copies the data if config.Immutable is false.

Additional Context (optional)

No response

Code Snippet (optional)

No response

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my suggestion prior to opening this one.
  • I understand that improperly formatted feature requests may be closed without explanation.
@leonklingele leonklingele changed the title 🚀 [v3 Feature]: "CopyBytes" and "CopyString" should only copy is "config.Immutable" is set 🚀 [v3 Feature]: "CopyBytes" and "CopyString" should only copy if "config.Immutable" is set Jan 6, 2023
@pjebs
Copy link
Contributor

pjebs commented Jan 7, 2023

I use the functions for non fiber purposes.

@ReneWerner87
Copy link
Member

Can you explain this more? The immutable switch should ensure that a copy is created whenever the functionality is used internally.

Here the focus this time is not necessarily on performance, but rather on handling strings as you would expect from golang.

@ReneWerner87 ReneWerner87 added the v3 label Jan 9, 2023
@leonklingele
Copy link
Member Author

For example the session middleware uses utils.CopyString to create a safe-copy of some cookie value:

id := c.Cookies(s.sessionName)
if len(id) > 0 {
return utils.CopyString(id)
}

As per its documentation, (*Ctx).Cookies tells you to do so:

fiber/ctx.go

Lines 495 to 496 in 61a3336

// Make copies or use the Immutable setting to use the value outside the Handler.
func (c *Ctx) Cookies(key string, defaultValue ...string) string {

However, when the Immutable config option is set, (*Ctx).Cookies already returns a safe-copy of the cookie value, making the session middleware effectively create a safe-copy of the safe-copy when Immutable = true:

This request basically boils down to providing handlers (or, more precisely, functions with access to a request's *Ctx) a way to skip the second safe-copy step, which might look as follows:

func (c *Ctx) CopyString(s string) string {
    if c.app.config.Immutable {
        // Input string "s" is already a copy and safe to use
        return s
    }
    return utils.CopyString(s)
}

@leonklingele leonklingele changed the title 🚀 [v3 Feature]: "CopyBytes" and "CopyString" should only copy if "config.Immutable" is set 🚀 [v3 Feature]: "CopyBytes" and "CopyString" should only copy if "config.Immutable" is false Feb 18, 2023
@leonklingele leonklingele changed the title 🚀 [v3 Feature]: "CopyBytes" and "CopyString" should only copy if "config.Immutable" is false 🚀 Feature: "CopyBytes" and "CopyString" should only copy if "config.Immutable" is false Feb 18, 2023
@ReneWerner87 ReneWerner87 added this to v3 Nov 22, 2023
@ReneWerner87 ReneWerner87 moved this to Todo in v3 Nov 22, 2023
@ReneWerner87 ReneWerner87 added this to the v3 milestone Nov 22, 2023
@nickajacks1
Copy link
Member

should an internal member function of the App object be used similar to app.getString? I would find it confusing if CopyString didn't actually copy.

@ReneWerner87
Copy link
Member

ReneWerner87 commented Jan 19, 2024

should an internal member function of the App object be used similar to app.getString? I would find it confusing if CopyString didn't actually copy.

Do you have a better name for this? When the immutable setting is on, its a copy otherwise its a reference

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

No branches or pull requests

4 participants