code review 13004046: encoding/xml: flush buffer after encoding token

encoding/xml: flush buffer after encoding token

[Dominik Honnef, 2013-08-17 05:12:07]

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

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-08-17 08:32:13]

All the tests already pass.
 On Aug 16, 2013 10:12 PM, <dominik.honnef@gmail.com> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> encoding/xml: flush buffer after encoding token
>
> Please review this at
https://codereview.appspot.**com/13004046/<https://codereview.appspot.com/130...
>
> Affected files:
>   M src/pkg/encoding/xml/marshal.**go
>
>
> Index: src/pkg/encoding/xml/marshal.**go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/xml/**marshal.go
> +++ b/src/pkg/encoding/xml/**marshal.go
> @@ -196,7 +196,6 @@
>                 p.WriteString("<!--")
>                 p.Write(t)
>                 p.WriteString("-->")
> -               return p.cachedWriteError()
>         case ProcInst:
>                 if t.Target == "xml" || !isNameString(t.Target) {
>                         return fmt.Errorf("xml: EncodeToken of ProcInst
> with invalid Target")
> @@ -219,7 +218,7 @@
>                 p.Write(t)
>                 p.WriteString(">")
>         }
> -       return p.cachedWriteError()
> +       return p.Flush()
>  }
>
>  type printer struct {
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>

[Dominik Honnef, 2013-08-17 17:16:30]

On 2013/08/17 08:32:13, bradfitz wrote:
> All the tests already pass.

There are no tests for this function, but I'm not the right one to write them.
To demonstrate the problem, however: http://play.golang.org/p/w0ruXyhgSV will
not produce any output.

[adg, 2013-08-18 22:25:01]

Here's your test (not tested ;-):

func TestEncodeTokenFlush(t *testing.T) {
var b bytes.Buffer
enc := xml.NewEncoder(&b)
 enc.EncodeToken(xml.StartElement{Name: xml.Name{Local: "some-tag"}})
if g, w := b.Len(), len("<some-tag>"); g != w {
 t.Errorf("Encoder wrote %v bytes, want %v", g, w)
}
}

[Dominik Honnef, 2013-08-18 22:41:59]

Hello rsc@golang.org, bradfitz@golang.org, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[adg, 2013-08-18 23:30:07]

https://codereview.appspot.com/13004046/diff/18001/src/pkg/encoding/xml/marsh...
File src/pkg/encoding/xml/marshal_test.go (right):

https://codereview.appspot.com/13004046/diff/18001/src/pkg/encoding/xml/marsh...
src/pkg/encoding/xml/marshal_test.go:1081: 
delete blank line

https://codereview.appspot.com/13004046/diff/18001/src/pkg/encoding/xml/marsh...
src/pkg/encoding/xml/marshal_test.go:1084: if g, w := buf.Len(),
len("<some-tag>"); g != w {
actually you should probably just compare buf.String() with "<some-tag>"

[Dominik Honnef, 2013-08-18 23:39:11]

PTAL

https://codereview.appspot.com/13004046/diff/18001/src/pkg/encoding/xml/marsh...
File src/pkg/encoding/xml/marshal_test.go (right):

https://codereview.appspot.com/13004046/diff/18001/src/pkg/encoding/xml/marsh...
src/pkg/encoding/xml/marshal_test.go:1081: 
On 2013/08/18 23:30:08, adg wrote:
> delete blank line

Okay. Had it modelled after the test above this one.

https://codereview.appspot.com/13004046/diff/18001/src/pkg/encoding/xml/marsh...
src/pkg/encoding/xml/marshal_test.go:1084: if g, w := buf.Len(),
len("<some-tag>"); g != w {
On 2013/08/18 23:30:08, adg wrote:
> actually you should probably just compare buf.String() with "<some-tag>"

Done.

[adg, 2013-08-18 23:55:55]

Sorry, one last nit

https://codereview.appspot.com/13004046/diff/14002/src/pkg/encoding/xml/marsh...
File src/pkg/encoding/xml/marshal_test.go (right):

https://codereview.appspot.com/13004046/diff/14002/src/pkg/encoding/xml/marsh...
src/pkg/encoding/xml/marshal_test.go:1084: t.Errorf("Encoder wrote %#v, want
%#v", g, w)
use %q for printing quoted strings

[Dominik Honnef, 2013-08-18 23:58:48]

Told you I wasn't the right person ;-)

https://codereview.appspot.com/13004046/diff/14002/src/pkg/encoding/xml/marsh...
File src/pkg/encoding/xml/marshal_test.go (right):

https://codereview.appspot.com/13004046/diff/14002/src/pkg/encoding/xml/marsh...
src/pkg/encoding/xml/marshal_test.go:1084: t.Errorf("Encoder wrote %#v, want
%#v", g, w)
On 2013/08/18 23:55:55, adg wrote:
> use %q for printing quoted strings

Done.

[adg, 2013-08-19 00:05:09]

LGTM

[adg, 2013-08-19 00:10:16]

On 19 August 2013 09:58, <dominik.honnef@gmail.com> wrote:

> Told you I wasn't the right person ;-)


Nonsense. We all learn every day. :-)

[adg, 2013-08-19 00:16:01]

*** Submitted as https://code.google.com/p/go/source/detail?r=5db14b33d6ef ***

encoding/xml: flush buffer after encoding token

R=rsc, bradfitz, adg
CC=golang-dev
https://codereview.appspot.com/13004046

Committer: Andrew Gerrand <adg@golang.org>

[rsc, 2013-09-03 20:30:07]

This is wrong and should be undone.

Flushing after every token negates nearly the entire advantage of buffering
writes in the first place.

Russ

[Dominik Honnef, 2013-09-03 20:38:38]

On 2013/09/03 20:30:07, rsc wrote:
> This is wrong and should be undone.
> 
> Flushing after every token negates nearly the entire advantage of buffering
> writes in the first place.
> 
> Russ

Are you opposed to adding a method to explicitly flush? My concrete problem is
that I am sending tokens down an XML stream (XMPP) and I need an
xml.StartElement to go down the wire as I send it.

[Dominik Honnef, 2013-09-03 20:52:14]

On 2013/09/03 20:38:38, Dominik Honnef wrote:
> On 2013/09/03 20:30:07, rsc wrote:
> > This is wrong and should be undone.
> > 
> > Flushing after every token negates nearly the entire advantage of buffering
> > writes in the first place.
> > 
> > Russ
> 
> Are you opposed to adding a method to explicitly flush? My concrete problem is
> that I am sending tokens down an XML stream (XMPP) and I need an
> xml.StartElement to go down the wire as I send it.

Actually, looking at it, how *do* you ensure that the buffer is flushed at any
given time? If all I am doing is using EncodeToken, I won't know how much of it
has reached the io.Writer of the Encoder.

[adg, 2013-09-04 00:44:49]

On 4 September 2013 06:30, Russ Cox <rsc@golang.org> wrote:

> This is wrong and should be undone.
>
> Flushing after every token negates nearly the entire advantage of
> buffering writes in the first place.
>

Without some change it is impossible to use EncodeToken correctly.

EncodeToken is not used by other parts of package xml. The buffering in the
encoder is obviously useful for the implementation of the Encode method,
but users of EncodeToken should perhaps be doing their own buffering anyway.

If you don't like this change, then we should add a Flush method to
Encoder, as Dominik suggests.

[rsc, 2013-09-05 16:03:28]

On Tue, Sep 3, 2013 at 8:44 PM, Andrew Gerrand <adg@golang.org> wrote:

>
> On 4 September 2013 06:30, Russ Cox <rsc@golang.org> wrote:
>
>> This is wrong and should be undone.
>>
>> Flushing after every token negates nearly the entire advantage of
>> buffering writes in the first place.
>>
>
> Without some change it is impossible to use EncodeToken correctly.
>

That's not strictly true. If you use it during the implementation of a
custom MarshalXML method, it works correctly without the flush, because
there will be a flush when the whole XML stream is done.

EncodeToken is not used by other parts of package xml. The buffering in the
> encoder is obviously useful for the implementation of the Encode method,
> but users of EncodeToken should perhaps be doing their own buffering anyway.
>
> If you don't like this change, then we should add a Flush method to
> Encoder, as Dominik suggests.
>

I agree that there should be a Flush.

Russ