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

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

Closed
3 tasks done
sujit-baniya opened this issue Nov 7, 2024 · 10 comments
Closed
3 tasks done

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

sujit-baniya opened this issue Nov 7, 2024 · 10 comments

Comments

@sujit-baniya
Copy link
Contributor

Bug Description

Video:

Screen.Recording.2024-11-07.at.10.00.08.mov
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:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my problem prior to opening this one.
  • I understand that improperly formatted bug reports may be closed without explanation.
@ReneWerner87
Copy link
Member

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

@gaby
Copy link
Member

gaby commented Nov 7, 2024

@ReneWerner87 Yes, it was added here with a big warning. #3074

@ReneWerner87
Copy link
Member

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

@gaby
Copy link
Member

gaby commented Nov 7, 2024

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

@ReneWerner87
Copy link
Member

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

@gaby
Copy link
Member

gaby commented Nov 7, 2024

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
Copy link
Member

ReneWerner87 commented Nov 7, 2024

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.

@sujit-baniya sujit-baniya changed the title 🐛 [Bug]: RebuidTree randomly causing nil reference issue 🐛 [Bug]: RebuildTree randomly causing nil reference issue Nov 7, 2024
@gaby
Copy link
Member

gaby commented Nov 7, 2024

@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
Copy link
Contributor Author

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
Copy link
Member

gaby commented Nov 8, 2024

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

See #3098

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