gofiber / fiber

⚡️ Express inspired web framework written in Go
https://gofiber.io
MIT License
34.17k stars 1.68k forks source link

🐛 [Bug]: RebuildTree randomly causing nil reference issue #3190

Closed sujit-baniya closed 3 weeks ago

sujit-baniya commented 3 weeks ago

Bug Description

Video:

https://github.com/user-attachments/assets/235a1bb4-00b9-45ce-a9e3-cd8eef2f19b8

package main

import (
    "github.com/gofiber/fiber/v3"
)

func main() {
    app := fiber.New()

    app.Get("/", func(c fiber.Ctx) error {
        app.Get("/hello", func(ctx fiber.Ctx) error {
            return c.SendString("Hello, World 👋!")
        })
        app.RebuildTree()
        return c.SendString("Added")
    })
    app.Listen(":3000")
}

In this code, I just tried to add a new route and rebuild the route stack. When trying to visit the newly added route, the error is thrown which is random in nature.

Error:

    _______ __             
   / ____(_) /_  ___  _____
  / /_  / / __ \/ _ \/ ___/
 / __/ / / /_/ /  __/ /    
/_/   /_/_.___/\___/_/          v3.0.0-beta.3
--------------------------------------------------
INFO Server started on:         http://127.0.0.1:3000 (bound on host 0.0.0.0 and port 3000)
INFO Total handlers count:      1
INFO Prefork:                   Disabled
INFO PID:                       45679
INFO Total process count:       1

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x59b0037]

goroutine 20 [running]:
github.com/gofiber/fiber/v3.(*DefaultCtx).SendString(0xc0001b0018?, {0x59c39c9?, 0xc0001b0000?})
        /Users/sujit/Sites/fiber/ctx.go:1657 +0x17
main.main.func1.1({0x5a80200?, 0xc000090190?})
        /Users/sujit/Sites/fiber/examples/main.go:12 +0x2b
github.com/gofiber/fiber/v3.(*App).next(0xc0000e2a08, 0xc000118008)
        /Users/sujit/Sites/fiber/router.go:189 +0x1e2
github.com/gofiber/fiber/v3.(*App).requestHandler(0xc0000e2a08, 0x579a3af?)
        /Users/sujit/Sites/fiber/router.go:237 +0x2f3
github.com/valyala/fasthttp.(*Server).serveConn(0xc0000ac488, {0x5ad9ff8, 0xc0000a00e8})
        /Users/sujit/go/pkg/mod/github.com/valyala/fasthttp@v1.57.0/server.go:2385 +0xed1
github.com/valyala/fasthttp.(*workerPool).workerFunc(0xc0000fa090, 0xc0000e4200)
        /Users/sujit/go/pkg/mod/github.com/valyala/fasthttp@v1.57.0/workerpool.go:225 +0x92
github.com/valyala/fasthttp.(*workerPool).getCh.func1()
        /Users/sujit/go/pkg/mod/github.com/valyala/fasthttp@v1.57.0/workerpool.go:197 +0x32
created by github.com/valyala/fasthttp.(*workerPool).getCh in goroutine 1
        /Users/sujit/go/pkg/mod/github.com/valyala/fasthttp@v1.57.0/workerpool.go:196 +0x194
exit status 2

Error Code:

// SendString sets the HTTP response body for string types.
// This means no type assertion, recommended for faster performance
func (c *DefaultCtx) SendString(body string) error {
    c.fasthttp.Response.SetBodyString(body)

    return nil
}

How to Reproduce

Steps to reproduce the behavior:

Run following code, at random there should be such error:

package main

import (
    "github.com/gofiber/fiber/v3"
)

func main() {
    app := fiber.New()

    app.Get("/", func(c fiber.Ctx) error {
        app.Get("/hello", func(ctx fiber.Ctx) error {
            return c.SendString("Hello, World 👋!")
        })
        app.RebuildTree()
        return c.SendString("Added")
    })
    app.Listen(":3000")
}

Expected Behavior

Application should have executed without any issue

Fiber Version

v3.0.0-beta.3

Code Snippet (optional)

package main

import "github.com/gofiber/fiber/v3"
import "log"

func main() {
  app := fiber.New()

  // Steps to reproduce

  log.Fatal(app.Listen(":3000"))
}

Checklist:

ReneWerner87 commented 3 weeks ago

Have we changed our strategy? Is adding a route after the server start possible in v3, didn't know that.

gaby commented 3 weeks ago

@ReneWerner87 Yes, it was added here with a big warning. https://github.com/gofiber/fiber/pull/3074

ReneWerner87 commented 3 weeks ago

Ok then its a bug. Seems that this method is not 100% thread safe.

gaby commented 3 weeks ago

It is not, thus why the warning. Basically that last test hit the server while the route was being added to the Router.

ReneWerner87 commented 3 weeks ago

Or the access for the routing items is not thread safe, but adding a mutex here will slow down it for everyone.

gaby commented 3 weeks ago

Or the access for the routing items is not thread safe, but adding a mutex here will slow down it for everyone.

Yes correct.

ReneWerner87 commented 3 weeks ago

However, we could make a switch in the config called dynamicRouting and only use the mutex if it is set to true.

Then the others would not run slower and you could only use mutex in the routing process and also in rebuildRouting if this is set. This would also make rebuild tree faster for those who do not use dynamic routing.

gaby commented 3 weeks ago

@sujit-baniya Found the problem in your code, the handler being created inside your first handler is using the context from the parent handler. The context is only valid during the duration of the request.

Change your code to this:

package main

import (
    "github.com/gofiber/fiber/v3"
)

func main() {
    app := fiber.New()

    app.Get("/", func(c fiber.Ctx) error {
        app.Get("/hello", func(ctx fiber.Ctx) error {
            // Use ctx not c here
            return ctx.SendString("Hello, World 👋!")
        })
        app.RebuildTree()
        return c.SendString("Added")
    })
    app.Listen(":3000")
}

I added a comment where the issue was.

sujit-baniya commented 3 weeks ago

Ah! ok... I'm attempting few more times before closing this issue.

By the way, I was expecting this code to work but after visiting /updated I expect to get "Bye, World 👋!" but received "Hello, World 👋!"


package main

import (
    "github.com/gofiber/fiber/v3"
)

func main() {
    app := fiber.New()

    app.Get("/", func(c fiber.Ctx) error {
        app.Get("/hello", func(ctx fiber.Ctx) error {
            // Use ctx not c here
            return ctx.SendString("Hello, World 👋!")
        })
        app.RebuildTree()
        return c.SendString("Added")
    })

    app.Get("/updated", func(c fiber.Ctx) error {
        app.Get("/hello", func(ctx fiber.Ctx) error {
            // Use ctx not c here
            return ctx.SendString("Bye, World 👋!")
        })
        app.RebuildTree()
        return c.SendString("Updated")
    })
    app.Listen(":3000")
}
gaby commented 3 weeks ago

@sujit-baniya You can't overwrite or remove routes.

See https://github.com/gofiber/fiber/issues/3098