Remove buffer pool for HTML render

[nsmith5]

http.ResponseWriter (the most common io.Writer passed to Render) is already buffered and this buffer pool simply lead to memory bloat under certain conditions. This work fixes #85.

As an example of the memory savings here is memory usage on my Gitea instance before and after this change:

Before

After

[lunny]

The buffer should be the io.Writer's responsibility. As a common usage, bufio.NewWriter(w), but this also should be invoked out of Render.

And in fact, the whole buffer.go could be removed.

[nsmith5]

Agreed. I left the buffer.go because the bufferpool's implementation is exported so it would be break the library API to remove, but could remove as well if its desired.

[zeripath]

Agreed, it should be up to the end-user if they want to buffer.

[KN4CK3R]

The change prevents the HTTP header from being set. If w is a http.ResponseWriter the first call to Write inside h.Templates.ExecuteTemplate(w, h.Name, binding) will set and write the header data. So h.Head.Write(hw) does nothing afterwards.

[zeripath]

That's up to the end user surely?

This function is not accepting a http.ResponseWriter it's accepting an io.Writer - the semantics of an http.ResponseWriter requiring WriteHeader() before Write(...) are not expressed in this and assuming that every io.Writer is or could be an http.ResponseWriter is a bit different.

This buffer is unbounded and worse the unbounded buffer is then kept around - potentially to be created 64 times

---

If you want to buffer before sending the status you should buffer. However, there are essentially two options:

• Size limit the results from this HTML function when buffering
• Only buffer the first n bytes, send the header if the first n bytes have succeeded then the buffer contents and reset buffer and so on. If an error occurs you then need to handle dangling markup.

Buffering needs to be size limited especially if you're rendering uncontrolled content.

[nsmith5]

Ahhh I see. The buffer pool isn't about write buffering, its about setting status after templating. I guess this makes the options more tricky. The buffer pool leads to growing allocations, but if a new bytes.Buffer is allocated for each template call the GC cost might get spendy...

Not sure the best option here.

[unrolled]

At first glance I was excited as this would be a great memory saver! But I don't think we can remove the buffer pool without breaking existing functionality.

We could add a new option that allows the Render func to skip the buffer and go strait into the provided io.Writer... something users can opt into? In the future we can roll a new version with breaking changes and enable the skip buffer option by default.

Thoughts?

[nsmith5]

If this buffer pooling is kept I can think of a couple improvements

• Use a sized buffer pool like this one to keep memory consumption fixed and to GC buffers that exceed the size limit after use
• Allow the bufferpool to be configurable so that the hidden default buffer pool can be modified (i.e if I know I'm most of my templates are under 100KiB, then I'd like to use a smaller max buffer size. If I know I've got a large system load and can increase the size of the pool to limit gc events under high concurrency)

More broadly though, I think this library might be improved by seperating these HTTP concerns from templating concerns. This might sacrifice the nice one-liners in the examples, but the behaviour would likely be less surprising.

[unrolled]

Love the improvements! Do you want to try implementing the changes in this PR?

I also agree that separating the concerns would improve the library and should be done. I'm going to start a "Version 2" ticket where we can document ideas and goals for the next version. I've been wanting to clean this package up for a while now... some of work-around code for blocks needs to go 🥾

[nsmith5]

Sweet happy to implement those :D I'll probably just open up a new PR if that works for you. I think we can close this one

[nsmith5]

This change breaks HTTP status updates. Going to implement a better and more configurable buffer pool instead.