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]: Wrong handlers execution order in some mount cases #2460

Closed
3 tasks done
sadfun opened this issue May 13, 2023 · 2 comments · Fixed by #2463
Closed
3 tasks done

🐛 [Bug]: Wrong handlers execution order in some mount cases #2460

sadfun opened this issue May 13, 2023 · 2 comments · Fixed by #2463

Comments

@sadfun
Copy link
Contributor

sadfun commented May 13, 2023

Bug Description

In some rare cases, fiber can wrongly build app.treeStack, breaking the execution chain. For example, it can call a handler from a mounted app before it calls a middleware specified at the top level earlier.

How to Reproduce

In attached code snippet, fiber thinks that middleware printing "i'm ignored!" should be executed after /methods/test handler.

I think there are many more cases when this happens, but for now I'm able to reproduce only this one.

Expected Behavior

Handlers execute in correct order.

Fiber Version

2.45.0

Code Snippet (optional)

package main

import (
	"fmt"
	"github.com/gofiber/fiber/v2"
	"net/http"
)

func getApi() *fiber.App {
	var API = fiber.New()

	methods := API.Group("/methods")

	methods.Get("/test", func(c *fiber.Ctx) error {
		if c.Locals("hello") != "world" {
			panic("hello is not world")
		}
		return c.SendStatus(http.StatusOK)
	})

	return API
}

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

	app.Use(func(c *fiber.Ctx) error {
		return c.Next()
	})

	app.Mount("/ws", fiber.New())

	app.Use(func(c *fiber.Ctx) error {
		return c.Next()
	})

	app.Get("/status", func(c *fiber.Ctx) error {
		return c.SendString("ok")
	})

	app.Use(func(c *fiber.Ctx) error {
		fmt.Println("i'm ignored!")
		c.Locals("hello", "world")
		return c.Next()
	})

	app.Mount("", getApi())

	test(app)
}

func test(app *fiber.App) {
	req, _ := http.NewRequest("GET", fmt.Sprintf("/methods/test"), nil)

	app.Test(req)
}

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 ReneWerner87 self-assigned this May 15, 2023
@ReneWerner87
Copy link
Member

@sadfun
big thanks for the good example

it really seems to be a bug

i assume it is a problem when i replace the mounted app

fiber/mount.go

Lines 214 to 218 in c56b4e6

newStack := make([]*Route, len(app.stack[m])+len(subRoutes)-1)
copy(newStack[:i], app.stack[m][:i])
copy(newStack[i:i+len(subRoutes)], subRoutes)
copy(newStack[i+len(subRoutes):], app.stack[m][i+1:])
app.stack[m] = newStack

will fix it in the next days and release it on friday

@ReneWerner87
Copy link
Member

the problem is the positions i created when replacing the mounted app
image

and after that they are sorted
image

ReneWerner87 added a commit that referenced this issue May 17, 2023
* 🐛 [Bug-fix]: Wrong handlers execution order in some mount cases #2460

* 🐛 [Bug-fix]: Wrong handlers execution order in some mount cases #2460

* 🐛 [Bug-fix]: Wrong handlers execution order in some mount cases #2460

* [Bug-fix]: Wrong handlers execution order in some mount cases #2460

* [Bug-fix]: Wrong handlers execution order in some mount cases #2460
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants