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

Strange behavior with groups + mw #96

Closed
alehano opened this issue Jun 9, 2015 · 3 comments
Closed

Strange behavior with groups + mw #96

alehano opened this issue Jun 9, 2015 · 3 comments
Assignees
Labels
Milestone

Comments

@alehano
Copy link

alehano commented Jun 9, 2015

I have several groups and some groups use specific middlewares.
But I noticed that in some combination of mw some groups start using middlewares from another groups.

For example:

files:

/static/test.txt
main.go

code:

package main

import (
    "log"

    "github.com/labstack/echo"
    mw "github.com/labstack/echo/middleware"
)

func main() {

    e := echo.New()
    e.Use(mw.Logger())
    e.Use(mw.Recover())
    e.Use(mw.Gzip())

    static := e.Group("/static")
    static.Use(MaxAgeMonth)
    static.ServeDir("/", "static")

    e.Get("/", func(c *echo.Context) error {
        return c.String(200, "Index")
    })

    admStatic := e.Group("/admin/static")
    admStatic.Use(MaxAgeMonth)
    admStatic.ServeDir("/", "admin/static")

    a := e.Group("/admin")
    a.Use(func(c *echo.Context) error {
        log.Println("Admin")
        return nil
    })
    a.Get("/info", func(c *echo.Context) error {
        return c.String(200, "OK")
    })

    e.Run(":8085")

}

func MaxAgeMonth(c *echo.Context) error {
    c.Response().Header().Add("Cache-Control", "max-age=2592000, public")
    return nil
}

So, if you open http://127.0.0.1:8085/static/test.txt
You'll see "Admin" in console. It means that /static/test.txt use another middleware which is in "/admin" group.

But if I remove e.Use(mw.Recover()) or e.Use(mw.Gzip()) everything works ok.
It's really odd.

@vishr
Copy link
Member

vishr commented Jun 9, 2015

@alehano I don't see this behavior after I fixed #95, can you confirm?

@vishr vishr added the bug label Jun 9, 2015
@vishr vishr self-assigned this Jun 9, 2015
@vishr vishr added this to the v0.1.0 milestone Jun 9, 2015
@alehano
Copy link
Author

alehano commented Jun 9, 2015

Yes, it's still there.

2015/06/09 20:22:24 Admin
2015/06/09 20:22:24 GET /static/test.txt 304 775.864µs 0
2015/06/09 20:22:25 Admin
2015/06/09 20:22:25 GET /static/test.txt 304 70.584µs 0
2015/06/09 20:22:27 Admin
2015/06/09 20:22:27 GET /static/test.txt 304 62.229µs 0

vishr added a commit that referenced this issue Jun 9, 2015
Signed-off-by: Vishal Rana <vr@labstack.com>
@vishr vishr closed this as completed Jun 9, 2015
@hasty
Copy link
Contributor

hasty commented Sep 3, 2015

This is because Group is relying on unreliable behavior in append. When a new group is created, it makes a copy of the parent echo; this is a shallow copy, though, so g.echo.middleware still points to the parent middleware. That's fine if you don't add any new middleware.

If you do, though, then it's dependent on whether or not the slice has capacity for the new middleware. If it doesn't, then append grows the slice and returns a new pointer to the larger slice, which contains all the parent middleware and the new middleware; this is fine. If you're unlucky and there's enough capacity, it just adds the new middleware to the slice, affecting both the parent echo and any other groups that are pointing at it.

A cheap fix is to just copy over the parent middleware into a new slice the first time you call Use() on Group. This got it working again for me. However, there are other ways that yield different results; for example, Group might keep its own mw list without copying, but then always iterate over the parent middleware before its own (or vice versa, as the case may be). This would allow you to add middleware to the parent echo after creating the group, which may be more like expected behavior.

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

No branches or pull requests

3 participants