gernest / utron

A lightweight MVC framework for Go(Golang)
MIT License
2.22k stars 149 forks source link

Highload bug #67

Closed co11ter closed 8 years ago

co11ter commented 8 years ago

Hi!

I write the code:

type info struct {
    c http.ResponseWriter
}

type Test struct {
    *utron.BaseController
}

func (t *Test) Index() {
    fmt.Println(info{t.Ctx.Response()})
    t.Ctx.Redirect("http://123123123.12", http.StatusFound)
}

func NewTest() *Test {
    return &Test{}
}

send many concurent requests to /test:

siege -f urls.txt -c 10 -t30s -i

and have error:

{0xc420691ee0}
{0xc4207ca000}
{0xc4207ca0d0}
{0xc4207ca1a0}
{0xc4207ca270}
{0xc4207ca340}
{0xc4206a4d00}
{0xc4206a4d00}
fatal error: concurrent map writes

goroutine 794 [running]:
runtime.throw(0x9ef0e2, 0x15)
    /usr/lib/go/src/runtime/panic.go:566 +0x95 fp=0xc420505220 sp=0xc420505200
runtime.mapassign1(0x98f7a0, 0xc4207b8c00, 0xc420505330, 0xc420505340)
    /usr/lib/go/src/runtime/hashmap.go:458 +0x8ef fp=0xc420505308 sp=0xc420505220
net/textproto.MIMEHeader.Set(0xc4207b8c00, 0x9e551b, 0x8, 0x9edaf9, 0x13)
    /usr/lib/go/src/net/textproto/header.go:22 +0xca fp=0xc420505368 sp=0xc420505308
net/http.Header.Set(0xc4207b8c00, 0x9e551b, 0x8, 0x9edaf9, 0x13)
    /usr/lib/go/src/net/http/header.go:31 +0x53 fp=0xc4205053a0 sp=0xc420505368
net/http.Redirect(0xe63320, 0xc4206a4d00, 0xc4207b3680, 0x9edaf9, 0x13, 0x12e)
    /usr/lib/go/src/net/http/server.go:1819 +0xd7 fp=0xc420505488 sp=0xc4205053a0
github.com/gernest/utron.(*Context).Redirect(0xc420174e70, 0x9edaf9, 0x13, 0x12e)
    /home/coder/go/src/github.com/gernest/utron/context.go:189 +0x5f fp=0xc4205054c8 sp=0xc420505488

...

Two parallel routines get the same context. Maybe action signature should be

func (c *SomeController) Action(ctx *utron.Context) {}

instead to save the context into the controller??

gernest commented 8 years ago

@co11ter Thanks for pointing this out.

I indeed the design had some issues. I'm open to suggestion, I will have to rewrite the implementation and possible change the API if possible.

Any input is welcome. You can help if you don't mind.

gernest commented 8 years ago

Someone also pointed this out #57

gernest commented 8 years ago

Problems seems to arise here https://github.com/gernest/utron/blob/master/routes.go#L313-L352

I think I might have ideas on how to fix this without changing of the API

co11ter commented 8 years ago

I fix it for me like https://github.com/co11ter/utron/blob/master/routes.go#L382-L398, but I don't think that it's "true way". Maybe we have to create new controller per request?

co11ter commented 8 years ago

Another fix https://github.com/co11ter/utron/commit/fb99f6f61fc79d29240433a96e6622acaed84915 but it change API:

type SomeController struct {
    *utron.BaseController
}

func NewSomeController() utron.Controller {
     return &SomeController{&utron.BaseController{}}
}

utron.RegisterController(NewSomeController)

I can send pull request if you want.

gernest commented 8 years ago

@co11ter cool. Did you test the first one?

I don't think changing the API will be wise for now! Please send the PR, then we can maybe discuss there if any changes need to be made!

gernest commented 8 years ago

I cant spopt the difference , please Open the PR so I can check the diff!

gernest commented 8 years ago

I think, for now the best way will be to create a new Controller on each request.

gernest commented 8 years ago

@co11ter please try to tun the example again to see if this bug is gone for good

co11ter commented 8 years ago

yes, it fixed!

gernest commented 8 years ago

@co11ter awesome. Thanks.