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

Writing directly into the response writer does not trigger response before hook functions. #2545

Closed
3 tasks done
SKFrozenCloud opened this issue Nov 12, 2023 · 6 comments
Closed
3 tasks done

Comments

@SKFrozenCloud
Copy link

SKFrozenCloud commented Nov 12, 2023

Issue Description

When we execute a template with html/template and write it directly into the response writer, before hooks on the response do not run.

Just to clarify, I am a beginner.
I would use the workaround method but I assume it uses more memory.
Is this an actual fixable and relevant bug?
Or is the "workaround" method the way to actually do it?

Checklist

  • Dependencies installed
  • No typos
  • Searched existing issues and docs

Expected behaviour

Before hook should run when we write directly into the response writer.

Actual behaviour

Before hook does not run when we write directly into the response writer.

Steps to reproduce

Copy the code below and try the normal route, the bug route and the workaround route.

Working code to debug

package main

import (
	"bytes"
	"fmt"
	"html/template"
	"net/http"

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

func main() {
	e := echo.New()
	e.Use(middleware.Logger())

	// Our response before hook function
	e.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			c.Response().Before(func() {
				fmt.Printf("\nBefore hook did run\n")
			})

			return next(c)
		}
	})

	e.GET("/", func(c echo.Context) error {
		// Using built-in response functions does trigger the response-before-hook
		return c.String(http.StatusOK, "Hello, World!")
	})

	e.GET("/bug", func(c echo.Context) error {
		htmlTemplate := "Hello Bug!"
		tmpl, err := template.New("htmlTemplate").Parse(htmlTemplate)
		if err != nil {
			panic(err)
		}

		// Bug is here, where we directly write the html template into the reponse writer
		// The response-before-hook never runs
		err = tmpl.Execute(c.Response().Writer, nil)
		if err != nil {
			c.Logger().Error(err)
			return err
		}

		return nil
	})

	e.GET("/workaround", func(c echo.Context) error {
		htmlTemplate := "Hello Workaround!"

		tmpl, err := template.New("htmlTemplate").Parse(htmlTemplate)
		if err != nil {
			panic(err)
		}

		// Workaround is to write the rendered html into a new buffer and then pass that to a built-in return-response-function
		// I assume this would use more memory since we need a new buffer instead of writing directly into the response writer?
		var htmlBuffer bytes.Buffer
		err = tmpl.Execute(&htmlBuffer, nil)
		if err != nil {
			c.Logger().Error(err)
			return err
		}

		return c.HTMLBlob(http.StatusOK, htmlBuffer.Bytes())
	})

	e.Logger.Fatal(e.Start(":1323"))
}

Version/commit

v4.11.3

@aldas
Copy link
Contributor

aldas commented Nov 12, 2023

You could use err = tmpl.Execute(c.Response(), nil) instead of err = tmpl.Execute(c.Response().Writer, nil) as echo.Response implements Writer interface.

With err = tmpl.Execute(c.Response(), nil) beforeFuncs will be called.


In regards c.Response().Writer,
This is working as intended. Before functions are called only once (when response has not been "commited"), at the time when the headers are written to the response. With c.Response().Writer, you are accessing original writer that echo.Response is meant to wrap thus forgiving all "magick" that Echo response has.

Here:

echo/response.go

Lines 54 to 65 in 584cb85

func (r *Response) WriteHeader(code int) {
if r.Committed {
r.echo.Logger.Warn("response already committed")
return
}
r.Status = code
for _, fn := range r.beforeFuncs {
fn()
}
r.Writer.WriteHeader(r.Status)
r.Committed = true
}

@aldas aldas closed this as completed Nov 12, 2023
@aldas
Copy link
Contributor

aldas commented Nov 12, 2023

p.s. create your templates out of handler if they are not changing. That way you avoid overhead of parsing that template on each request

	// avoid parsing template in every request
	tmpl := template.Must(template.New("htmlTemplate").Parse("Hello Bug!"))
	
	e.GET("/bug", func(c echo.Context) error {
		if err := tmpl.Execute(c.Response(), nil); err != nil {
			c.Logger().Error(err)
			return err
		}
		return nil
	})

@SKFrozenCloud
Copy link
Author

Thank you.

Is saving it first to bytes.Buffer bad? More saving and copying?

Which method between the "workaround" method and the solution you posted do you think is the "best practice"?

@aldas
Copy link
Contributor

aldas commented Nov 12, 2023

Okay, you second argument to tmpl.Execute is nil. Does this probably means that you are outputting totally static HTML? nothing is there changing? If it is so then you do not need templates at all. Just write your HTML string out and you are good to go.

templates are most useful for cases when your output is dynamic. Although maybe you are using templates for convenience because they have ability to "render" subtemplates/fragments into single output. In that case templates are useful.

@SKFrozenCloud
Copy link
Author

The code was a minimal code to reproduce the bug.

But I will render it to a buffer then use the built-in response function.
I had some issues when writing directly to the response writer and then setting the header manually because the header had already been written once.

@aldas
Copy link
Contributor

aldas commented Nov 12, 2023

You can not set headers after the response body (at least 1 byte) is sent to the client. This is because headers come before body in HTTP response. If your headers depend on the response body (etag header?) you probably can not avoid buffers in case the body is dynamic.

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

No branches or pull requests

2 participants