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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悶 Document clearly that properties of the request ctx are invalid after the handler returns #185

Closed
yossizahn opened this issue Feb 24, 2020 · 6 comments

Comments

@yossizahn
Copy link

@yossizahn yossizahn commented Feb 24, 2020

Fiber version/commit: Latest
Issue description
Not sure if this is intentional or somewhat of a bug.
Currently, many properties of the request context are returned as strings but are unsafe to persist after the handler has returned (due to unsafe conversion here).

Expected behavior
It should be possible to safely store request properties for reuse after the handler has returned.

Steps to reproduce
See code snippet. Send a few requests in quick succession, e.g.:

for (( c=1; c<=10; c++ )); do   curl "http://localhost:3000/$c";   done

The second fmt.Printf will not output the correct value.

Code snippet

package main
 
import (
        "fmt"
        "time"
 
        "github.com/gofiber/fiber"
)
 
func main() {
        app := fiber.New()
 
        app.Get("/:number", func(c *fiber.Ctx) {
                number := c.Params("number")
                go myfunc(number)
                c.Send(number)
        })
        app.Listen(3000)
}
func myfunc(number string) {
        fmt.Printf("number is %s \n", number)
        time.Sleep(1 * time.Second)
        fmt.Printf("number is now %s \n", number)
}
@yossizahn yossizahn added the bug label Feb 24, 2020
@welcome
Copy link

@welcome welcome bot commented Feb 24, 2020

Thanks for opening your first issue here! 馃帀 Be sure to follow the issue template!

@Fenny
Copy link
Member

@Fenny Fenny commented Feb 24, 2020

We use the same method as Fasthttp to convert strings and bytes without memory allocation, but proper use requires you to never change the original byte array, but that's a reasonable contract in many cases. I did not see many use-cases to persist values after returning from the handler, so this is an interesting topic.

@yossizahn
Copy link
Author

@yossizahn yossizahn commented Feb 25, 2020

It is true that it is possible to reproduce the same behavior in Fasthttp, but in Fasthttp it is more forgivable since a) it is clearly documented and b) there is no implicit contract implied in returning a []byte saying that it won't change under your feet.
It would seem to me that returning a string constitutes an implicit contract that it will follow string conventions such as being immutable.

@Fenny
Copy link
Member

@Fenny Fenny commented Feb 25, 2020

@yossizahn, you have a good point and we can provide an Immutable option so return values within the handler are immutable. Update the docs that by default all context values are valid until you return from the handler. This way we can keep zero memory allocation conversion enabled by default.

func main() {
  app := fiber.New(&fiber.Settings{
    Immutable: true, 
  })
  app.Get("/:number", func(c *fiber.Ctx) {
    number := c.Params("number") // immutable
    go func() {
      time.Sleep(1 * time.Second)
      fmt.Println("number: ", number)
    }()
  })
  app.Listen(3000)
}

@Fenny Fenny mentioned this issue Feb 25, 2020
@Fenny Fenny mentioned this issue Feb 26, 2020
@Fenny
Copy link
Member

@Fenny Fenny commented Feb 27, 2020

I'm closing this issue because it will be addressed in v1.8.0, feel free to re-open!

@calbot
Copy link

@calbot calbot commented Aug 14, 2021

I wish the strings returned from the context were wrapped in a new unsafestring struct so when you wrote the code you are forced to call getUnsafeString() or something so it's obvious what you're doing. That way I would pass around unsafestring into functions so I know the string is mutable! All the other go docs say strings are immutable.

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

No branches or pull requests

3 participants