martini-contrib / render

Martini middleware/handler for easily rendering serialized JSON, XML, and HTML template responses.
MIT License
245 stars 57 forks source link

use Write method #18

Closed NoahShen closed 10 years ago

NoahShen commented 10 years ago

Using Write method instead of io.Copy, make sure martini.ResponseWriter.Before method invoked

codegangsta commented 10 years ago

Doesn't io.Copy() call write on the io.Writer?

NoahShen commented 10 years ago

I used martini-contrib/sessions and martini-contrib/render at same time. I saw the sources of io.Copy, sessions and render. It should invoke BeforeFunction to write cookies into response when executing render.HTML(...) When i ran my code, it didn't write cookies when using io.Copy, but it worked when using write method. I am confused about this.

codegangsta commented 10 years ago

Hm. very interesting. I will have to look into this. Thanks for the report

codegangsta commented 10 years ago

What is the order of your middleware when you can reproduce the issue. This may be a middleware ordering issue

NoahShen commented 10 years ago

This is my code: Last line render.HTML(200, "dashboard", uInfo), didn't send cookies to browser


func StartServer() {
    gob.Register(&UserInfo{})

    m := martini.Classic()
    m.Use(render.Renderer(render.Options{
        Directory: "../../web/templates",
    }))

    store := sessions.NewCookieStore([]byte("secretDpGitLab"))
    store.Options(sessions.Options{
        MaxAge: int(constant.SessionMaxAge),
    })
    m.Use(sessions.Sessions(constant.SessionKey, store))

    m.Use(filter.LoginFilter())

    m.Group("/action", func(r martini.Router) {
        r.Post("/login", loginWithNameAndPwd)
        r.Get("/projects", getProject)
    })

    m.Run()

}

func loginWithNameAndPwd(r *http.Request, res http.ResponseWriter, params martini.Params, session sessions.Session, render render.Render, l *log.Logger) {
    err := r.ParseForm()
    if err != nil {
        l.Printf("ParseForm error: %v\n", err)
        render.HTML(200, "error", "login failed!")
        return
    }

    username := r.PostFormValue("username")
    password := r.PostFormValue("password")

    l.Printf("Start logging in, username = %s \n", username)
    gitlabClient, err := gogitlab.NewGitlabByLogin(constant.GitLabHost, constant.GitLabApiPath, username, password)
    if err != nil {
        l.Printf("Login error: %v\n", err)
        render.HTML(200, "error", "login failed!")
        return
    }
    gitUser, err := gitlabClient.CurrentUser()
    if err != nil {
        l.Printf("Get current error: %v\n", err)
        render.HTML(200, "error", "login failed!")
        return
    }
    uInfo := &UserInfo{&gitUser, gitlabClient}
    session.Set(constant.UserInfoKey, uInfo)
    render.HTML(200, "dashboard", uInfo)
}
codegangsta commented 10 years ago

What happens when render.Renderer is the last middleware (after sessions)?

Sent from my iPhone

On Apr 15, 2014, at 6:23 PM, Noah Shen notifications@github.com wrote:

This is my code: Last line render.HTML(200, "dashboard", uInfo), didn't send cookies to browser

func StartServer() { gob.Register(&UserInfo{})

m := martini.Classic()
m.Use(render.Renderer(render.Options{
    Directory: "../../web/templates",
}))

store := sessions.NewCookieStore([]byte("secretDpGitLab"))
store.Options(sessions.Options{
    MaxAge: int(constant.SessionMaxAge),
})
m.Use(sessions.Sessions(constant.SessionKey, store))

m.Use(filter.LoginFilter())

m.Group("/action", func(r martini.Router) {
    r.Post("/login", loginWithNameAndPwd)
    r.Get("/projects", getProject)
})

m.Run()

}

func loginWithNameAndPwd(r http.Request, res http.ResponseWriter, params martini.Params, session sessions.Session, render render.Render, l log.Logger) { err := r.ParseForm() if err != nil { l.Printf("ParseForm error: %v\n", err) render.HTML(200, "error", "login failed!") return }

username := r.PostFormValue("username")
password := r.PostFormValue("password")

l.Printf("Start logging in, username = %s \n", username)
gitlabClient, err := gogitlab.NewGitlabByLogin(constant.GitLabHost, constant.GitLabApiPath, username, password)
if err != nil {
    l.Printf("Login error: %v\n", err)
    render.HTML(200, "error", "login failed!")
    return
}
gitUser, err := gitlabClient.CurrentUser()
if err != nil {
    l.Printf("Get current error: %v\n", err)
    render.HTML(200, "error", "login failed!")
    return
}
uInfo := &UserInfo{&gitUser, gitlabClient}
session.Set(constant.UserInfoKey, uInfo)
render.HTML(200, "dashboard", uInfo)

} — Reply to this email directly or view it on GitHub.

unrolled commented 10 years ago

@NoahShen is there any update on this? I've tried to reproduce the problem, but I'm always getting the cookies sent through as expected.

NoahShen commented 10 years ago

Yes, it worked when render.Renderer was the last middleware. @codegangsta @unrolled

unrolled commented 10 years ago

I liked that this pull request brought consistency to the HTML function (it was the only one using io.Copy). My initial assumption was that io.Copy would be more efficient, but after bench marking the current implementation and pull request, I can say the r.Write(out.Bytes()) method is faster. Results below are an average of 5 runs each:

Method ns/op
io.Copy 602718.50
r.Write 598424.25