馃 [Question]: Timeout is not work in Fiber v2.42.0 #2364
Comments
|
Thanks for opening your first issue here! 馃帀 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
@hakankutluay pls help here ? we probably produced a breaking change in the last amendment after all because the behavior has changed #2090 before the usage was possible without using the context in the handler please test it yourself package main
import (
"log"
"time"
"github.com/gofiber/fiber/v2"
"github.com/gofiber/fiber/v2/middleware/timeout"
)
func main() {
app := fiber.New()
// Like this , the timeout does not work
app.Get("/abc", timeout.New(func(ctx *fiber.Ctx) error {
time.Sleep(2 * time.Second)
return nil
}, time.Millisecond*1000))
log.Fatal(app.Listen(":3000"))
}curl --location 'localhost:3000/abc' -ICurrent state (after Fiber v2.38.1): HTTP/1.1 200 OK
Date: Fri, 10 Mar 2023 08:44:49 GMTExpectation (works in Fiber v2.37.1): HTTP/1.1 408 Request Timeout
Date: Fri, 10 Mar 2023 08:50:44 GMT
Content-Type: text/plain; charset=utf-8
Content-Length: 15 |
|
@hakankutluay would like to restore the original functionality and provide a separate function for working with contexts as mentioned in the old pull request |
|
@ReneWerner87 sure, but the original functionality has race conditions and is not safe to use on production. The PR was for a new timeout functionality but we have decided to replace it after a discussion. If it's ok to have the original one and add new timeout as like |
|
@ReneWerner87 we have a workaround to make it work with func main() {
app := fiber.New()
// Like this , the timeout does not work
app.Get("/abc", timeout.New(func(ctx *fiber.Ctx) error {
time.Sleep(2 * time.Second)
return nil
}, time.Millisecond*1000))
log.Fatal(app.Listen(":3000"))
}Sleep with context timeout func main() {
app := fiber.New()
// Like this , the timeout does not work
app.Get("/abc", timeout.New(func(ctx *fiber.Ctx) error {
if err := sleepWithContext(c.UserContext(), 2 * time.Second, context.DeadlineExceeded); err != nil {
return fmt.Errorf("%w: execution error", err)
}
return nil
}, time.Millisecond*1000))
log.Fatal(app.Listen(":3000"))
}
func sleepWithContext(ctx context.Context, d time.Duration, te error) error {
timer := time.NewTimer(d)
select {
case <-ctx.Done():
if !timer.Stop() {
<-timer.C
}
return te
case <-timer.C:
}
return nil
} |
|
Can we provide something so that the old functionality behavior is restored without the consumers having to change anything (otherwise it is a breaking change if the behavior changes)? |
|
@ReneWerner87 I couldn't find a way to implement race condition-free timeout middleware that supports |
|
@hakankutluay can you create an example of the race conditions in code so i can recreate them |
|
@ReneWerner87 sure, server func main() {
app := fiber.New()
app.Use(recover.New(recover.Config{
EnableStackTrace: true,
}))
app.Use(logger.New())
// Like this , the timeout does not work
app.Get("/abc", timeout.New(func(ctx *fiber.Ctx) error {
time.Sleep(time.Millisecond * 2000)
return ctx.SendStatus(http.StatusNoContent)
}, time.Millisecond*1000))
log.Fatal(app.Listen(":3200"))
}run with go run -race ./main.gocall in parallel seq 1 200 | xargs -n1 -P20 curl "http://localhost:3200/abc"some of them will introduce race conditions |
|
Then let's mark the old function deprecated with reference to the new one and an example for the usage |
|
@hakankutluay back then the timeout middleware was a copy of the timeout handler, maybe i had forgotten something in the adaption ? or is this timeouthandler in use also with race conditions ? can you check this |
|
maybe the part with the check for the concurrency request count |
|
@ReneWerner87 I have checked Probably Or context has special field for timeout response as |
|
I don't think we need to provide an old broken way and a new way which is broken as well. Why can't we use |
|
By the way, the new implementation supports context cancelation, which is useful to cancel underlying operations like DB calls, http calls etc. that are not supported directly on |
|
@hakankutluay Can you create a pull request in fasthttp which allows this? |
|
@hakankutluay can you take a look at this ? |
|
@ReneWerner87 not yet, I'll do whenever I have a time |
Question Description
Timeout is not work in Fiber v2.42.0
Code Snippet (optional)
Checklist:
The text was updated successfully, but these errors were encountered: