Wrap http.ResponseWriter to greatly increase middleware/logging accesibilty #16

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
@zmarcantel

The returning status is hidden in http.ResponseWriter, but it's easy to wrap.

This follows Martini and other packages practice and allows giving access to many variables.
I have only included .Status() as an example, but Martini has body length and a few others.

There is also an example of an extended logger working as a middleware using the .Status() function to color output based on status code.
This examples could easily extend to other middleware that "fork" handling based on a specific status such as:

  1. monitoring
  2. logging
  3. intrusion detection
  4. internal health checks
  5. etc
@manucorporat

This comment has been minimized.

Show comment
Hide comment
@manucorporat

manucorporat Jul 2, 2014

Contributor

Nice! good job. We already experimented wrapping the http.ResponseWriter, but we experimented a couple of bugs. For example: when you call w.Write(buffer) this method could also change the status code, but the statusvariable won't. This small change also adds an small allocation overhead.

Definitely it's on the roadmap. I will review your pull request tomorrow.
Thank you in advance!

Contributor

manucorporat commented Jul 2, 2014

Nice! good job. We already experimented wrapping the http.ResponseWriter, but we experimented a couple of bugs. For example: when you call w.Write(buffer) this method could also change the status code, but the statusvariable won't. This small change also adds an small allocation overhead.

Definitely it's on the roadmap. I will review your pull request tomorrow.
Thank you in advance!

@zmarcantel

This comment has been minimized.

Show comment
Hide comment
@zmarcantel

zmarcantel Jul 2, 2014

Ah! Good catch. I'll see if I can get all the Write-esque methods patched up and resubmit

Ah! Good catch. I'll see if I can get all the Write-esque methods patched up and resubmit

@alexandernyquist

This comment has been minimized.

Show comment
Hide comment
@alexandernyquist

alexandernyquist Jul 2, 2014

Contributor

Wrapping it in some way would be very handy. At the moment, i'm experimenting with a GZIP-middleware but thats kind of hard to accomplish without a wrapped ResponseWriter.

I could wrap it myself inside the middleware but that feels wrong, and will definitely cause a performance hit.

Contributor

alexandernyquist commented Jul 2, 2014

Wrapping it in some way would be very handy. At the moment, i'm experimenting with a GZIP-middleware but thats kind of hard to accomplish without a wrapped ResponseWriter.

I could wrap it myself inside the middleware but that feels wrong, and will definitely cause a performance hit.

@manucorporat

This comment has been minimized.

Show comment
Hide comment
@manucorporat

manucorporat Jul 2, 2014

Contributor

Wrapping the ResponseWriter could be also used to implement a redis cache middleware for example!

We have to do it right, once the new allocator is ready, we will have much more flexibility to add features without degrading the performance. #24

Contributor

manucorporat commented Jul 2, 2014

Wrapping the ResponseWriter could be also used to implement a redis cache middleware for example!

We have to do it right, once the new allocator is ready, we will have much more flexibility to add features without degrading the performance. #24

@zmarcantel

This comment has been minimized.

Show comment
Hide comment
@zmarcantel

zmarcantel Jul 4, 2014

Sorry for the delay everyone :(
I've been living between a desk and airplanes for the past few days.

Let me know what you think and we can get it worked out much more quickly.

I haven't come up with an intuitive way to get past the allocation.
There has to be at least a uint8 to hold the status.
We can play with struct packing but I'm not seeing a performance difference between:

  1. without wrapping
  2. wrap with int (signed)
  3. wrap with uint8 (unsigned)

The benches were done fairly offhandedly on an i7 MacbookPro

Sorry for the delay everyone :(
I've been living between a desk and airplanes for the past few days.

Let me know what you think and we can get it worked out much more quickly.

I haven't come up with an intuitive way to get past the allocation.
There has to be at least a uint8 to hold the status.
We can play with struct packing but I'm not seeing a performance difference between:

  1. without wrapping
  2. wrap with int (signed)
  3. wrap with uint8 (unsigned)

The benches were done fairly offhandedly on an i7 MacbookPro

@manucorporat

This comment has been minimized.

Show comment
Hide comment
@manucorporat

manucorporat Jul 4, 2014

Contributor

@zmarcantel thanks to the new cache system, the context/responsewriter allocation is not going to be a problem any more. We could allocate 1MB and the performance would be still the same!!

The contexts and responseWriters are going to be allocated once and reused many times!
https://github.com/gin-gonic/gin/blob/develop/gin.go#L176-L183

The fix for this issue is already working in develop branch, but since it's a very important one, I am not going to close it yet.

Contributor

manucorporat commented Jul 4, 2014

@zmarcantel thanks to the new cache system, the context/responsewriter allocation is not going to be a problem any more. We could allocate 1MB and the performance would be still the same!!

The contexts and responseWriters are going to be allocated once and reused many times!
https://github.com/gin-gonic/gin/blob/develop/gin.go#L176-L183

The fix for this issue is already working in develop branch, but since it's a very important one, I am not going to close it yet.

@zmarcantel

This comment has been minimized.

Show comment
Hide comment
@zmarcantel

zmarcantel Jul 12, 2014

Will a develop branch be used in place of pull requests?
It seems my wrapper was used, modified (incompatibly), and then force pushed into master.
This is an odd workflow that creates essentially three HEADS.

For example:
I just ran a git pull, but my fork is no longer pointing at your merged develop branch. This makes future changes in this fork incompatible, removes credit for work anyone does, and creates a lot of work for maintainers.

If you wanted to change the names of the functions (only changes made) please accept this pull request and then immediately issue another commit changing them.... or ask the requester to do it so they get credit.

I realize it's too late now, but this is a little frustrating and deters further development.

Will a develop branch be used in place of pull requests?
It seems my wrapper was used, modified (incompatibly), and then force pushed into master.
This is an odd workflow that creates essentially three HEADS.

For example:
I just ran a git pull, but my fork is no longer pointing at your merged develop branch. This makes future changes in this fork incompatible, removes credit for work anyone does, and creates a lot of work for maintainers.

If you wanted to change the names of the functions (only changes made) please accept this pull request and then immediately issue another commit changing them.... or ask the requester to do it so they get credit.

I realize it's too late now, but this is a little frustrating and deters further development.

@manucorporat

This comment has been minimized.

Show comment
Hide comment
@manucorporat

manucorporat Mar 23, 2015

Contributor

@zmarcantel this was implemented a long time ago, but thanks for your work. I am sorry for the inconveniences I am going to add your username to the AUTHORS file.

Contributor

manucorporat commented Mar 23, 2015

@zmarcantel this was implemented a long time ago, but thanks for your work. I am sorry for the inconveniences I am going to add your username to the AUTHORS file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment