code review 4662091: http: support for periodic flushing in ReverseProxy

http: support for periodic flushing in ReverseProxy

Fixes issue 2012

[bradfitz, 2011-07-12 00:33:37]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dsymonds, 2011-07-12 00:38:25]

http://codereview.appspot.com/4662091/diff/4001/src/pkg/http/reverseproxy.go
File src/pkg/http/reverseproxy.go (right):

http://codereview.appspot.com/4662091/diff/4001/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:139: if f, ok := m.dst.(Flusher); ok {
Can you move this check up into ServeHTTP? That way you can avoid the overhead
of the ticker goroutine etc. if the ResponseWriter does not implement Flusher.

[bradfitz, 2011-07-12 00:41:07]

Hello golang-dev@googlegroups.com, dsymonds@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-07-12 00:41:22]

Sure, done.

On Mon, Jul 11, 2011 at 5:38 PM, <dsymonds@golang.org> wrote:

>
> http://codereview.appspot.com/**4662091/diff/4001/src/pkg/**
>
http/reverseproxy.go<http://codereview.appspot.com/4662091/diff/4001/src/pkg/http/reverseproxy.go>
> File src/pkg/http/reverseproxy.go (right):
>
> http://codereview.appspot.com/**4662091/diff/4001/src/pkg/**
>
http/reverseproxy.go#**newcode139<http://codereview.appspot.com/4662091/diff/4001/src/pkg/http/reverseproxy.go#newcode139>
> src/pkg/http/reverseproxy.go:**139: if f, ok := m.dst.(Flusher); ok {
> Can you move this check up into ServeHTTP? That way you can avoid the
> overhead of the ticker goroutine etc. if the ResponseWriter does not
> implement Flusher.
>
>
>
http://codereview.appspot.com/**4662091/<http://codereview.appspot.com/4662091/>
>

[dsymonds, 2011-07-12 00:46:49]

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go
File src/pkg/http/reverseproxy.go (right):

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:33: // FlushInterval optionally specifies the flush
s/optionally // (all of these are options).

add a mention as to what zero means.

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:116: type maxLatencyWriter struct {
I'm not a fan of the name, since it's a periodic flusher, rather than modifying
anything to do with the writing. I'm not fussed, though.

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:117: dst     io.Writer
define a trivial interface

type writeFlusher interface {
  io.Writer
  Flusher
}

and then you only need one var here.

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:128: if m.done == nil {
wouldn't it be simpler to initialise this up when the maxLatencyWriter is
constructed?

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:139: func (m *maxLatencyWriter) flush() {
I'd be tempted to inline this below.

[bradfitz, 2011-07-12 03:28:03]

Hello golang-dev@googlegroups.com, dsymonds@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-07-12 03:29:55]

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go
File src/pkg/http/reverseproxy.go (right):

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:33: // FlushInterval optionally specifies the flush
On 2011/07/12 00:46:49, dsymonds wrote:
> s/optionally // (all of these are options).
> 
> add a mention as to what zero means.

I've been led to believe the documentation rule is you say "optionally" (with an
obvious zero value) or explicitly declare the zero value's meaning, but not
both.

I suppose it can be argued that my zero value wasn't obvious.

Documented.

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:116: type maxLatencyWriter struct {
On 2011/07/12 00:46:49, dsymonds wrote:
> I'm not a fan of the name, since it's a periodic flusher, rather than
modifying
> anything to do with the writing. I'm not fussed, though.

It's private so it doesn't really matter, I figured.

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:117: dst     io.Writer
On 2011/07/12 00:46:49, dsymonds wrote:
> define a trivial interface
> 
> type writeFlusher interface {
>   io.Writer
>   Flusher
> }
> 
> and then you only need one var here.

Done.

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:128: if m.done == nil {
On 2011/07/12 00:46:49, dsymonds wrote:
> wouldn't it be simpler to initialise this up when the maxLatencyWriter is
> constructed?

*shrug*

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:139: func (m *maxLatencyWriter) flush() {
On 2011/07/12 00:46:49, dsymonds wrote:
> I'd be tempted to inline this below.

Done.

[dsymonds, 2011-07-12 03:34:10]

LGTM

This is enough polishing.

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go
File src/pkg/http/reverseproxy.go (right):

http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#...
src/pkg/http/reverseproxy.go:128: if m.done == nil {
On 2011/07/12 03:29:56, bradfitz wrote:
> On 2011/07/12 00:46:49, dsymonds wrote:
> > wouldn't it be simpler to initialise this up when the maxLatencyWriter is
> > constructed?
> 
> *shrug*

It seems that it's only the construction of m.done that needs the lock; without
it you preserve the concurrency-safety of the underlying writer/flusher. You can
leave it as you've got it, but it just seemed a bit unnecessary.

[bradfitz, 2011-07-12 03:56:27]

*** Submitted as http://code.google.com/p/go/source/detail?r=ee2ab6c0f36b ***

http: support for periodic flushing in ReverseProxy

Fixes issue 2012

R=golang-dev, dsymonds
CC=golang-dev
http://codereview.appspot.com/4662091

[bradfitz, 2011-07-12 03:59:28]

On Mon, Jul 11, 2011 at 8:34 PM, <dsymonds@golang.org> wrote:

> LGTM
>
> This is enough polishing.
>
>
>
> http://codereview.appspot.com/**4662091/diff/4002/src/pkg/**
>
http/reverseproxy.go<http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go>
> File src/pkg/http/reverseproxy.go (right):
>
> http://codereview.appspot.com/**4662091/diff/4002/src/pkg/**
>
http/reverseproxy.go#**newcode128<http://codereview.appspot.com/4662091/diff/4002/src/pkg/http/reverseproxy.go#newcode128>
> src/pkg/http/reverseproxy.go:**128: if m.done == nil {
> On 2011/07/12 03:29:56, bradfitz wrote:
>
>> On 2011/07/12 00:46:49, dsymonds wrote:
>> > wouldn't it be simpler to initialise this up when the
>>
> maxLatencyWriter is
>
>> > constructed?
>>
>
>  *shrug*
>>
>
> It seems that it's only the construction of m.done that needs the lock;
> without it you preserve the concurrency-safety of the underlying
> writer/flusher. You can leave it as you've got it, but it just seemed a
> bit unnecessary.


Fwiw, my reasoning is that if these were in separate files / packages, the
question would be: what are the essential parameters to the
MaxLatencyWriter, and what would be implementation details?

[dsymonds, 2011-07-12 04:11:41]

On Tue, Jul 12, 2011 at 1:59 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> Fwiw, my reasoning is that if these were in separate files / packages, the
> question would be: what are the essential parameters to the
> MaxLatencyWriter, and what would be implementation details?

In that case, though, you'd just write a constructor function.


Dave.