[protobuf] encoding: add protodelim package
Gopher Robot (Gerrit)
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Sachin Padmanabhan has uploaded this change for review.
encoding: add protodelim package
Inspired by discussion https://github.com/golang/protobuf/issues/1382
Change-Id: I30dc9bf9aa44e35cde8fb472c3b8b116d459714e
---
A encoding/protodelim/protodelim.go
A encoding/protodelim/protodelim_test.go
2 files changed, 147 insertions(+), 0 deletions(-)
diff --git a/encoding/protodelim/protodelim.go b/encoding/protodelim/protodelim.go
new file mode 100644
index 0000000..f474c38
--- /dev/null
+++ b/encoding/protodelim/protodelim.go
@@ -0,0 +1,81 @@
+// Package protodelim marshals and unmarshals varint size-delimited messages.
+package protodelim
+
+import (
+ "bufio"
+ "encoding/binary"
+ "io"
+
+ "google.golang.org/protobuf/proto"
+)
+
+// MarshalOptions is a configurable varint size-delimited marshaler.
+type MarshalOptions struct{ proto.MarshalOptions }
+
+// MarshalTo writes a varint size-delimited wire-format message to w.
+func (o MarshalOptions) MarshalTo(w io.Writer, m proto.Message) (int, error) {
+ msgBytes, err := o.MarshalOptions.Marshal(m)
+ if err != nil {
+ return 0, err
+ }
+
+ sizeBuf := make([]byte, binary.MaxVarintLen64)
+ numSizeBytes := binary.PutUvarint(sizeBuf[:], uint64(len(msgBytes)))
+
+ sizeWritten, err := w.Write(sizeBuf[:numSizeBytes])
+ if err != nil {
+ return sizeWritten, err
+ }
+ msgWritten, err := w.Write(msgBytes)
+ if err != nil {
+ return sizeWritten + msgWritten, err
+ }
+ return sizeWritten + msgWritten, nil
+}
+
+// MarshalTo writes a varint size-delimited wire-format message to w.
+func MarshalTo(w io.Writer, m proto.Message) (int, error) {
+ return MarshalOptions{}.MarshalTo(w, m)
+}
+
+// UnmarshalOptions configures a Decoder.
+type UnmarshalOptions struct{ proto.UnmarshalOptions }
+
+// Decoder is used to parse a stream of varint size-delimited wire-format messages.
+type Decoder struct {
+ opts UnmarshalOptions
+ bufReader *bufio.Reader
+}
+
+// NewDecoder returns a Decoder with default UnmarshalOptions.
+func NewDecoder(r io.Reader) *Decoder {
+ return NewDecoderWithOpts(r, UnmarshalOptions{})
+}
+
+// NewDecoderWithOpts returns a Decoder configured by o.
+func NewDecoderWithOpts(r io.Reader, o UnmarshalOptions) *Decoder {
+ return &Decoder{opts: o, bufReader: bufio.NewReader(r)}
+}
+
+// UnmarshalNext parses and consumes a varint size-delimited wire-format message
+// from the underlying Reader.
+// The provided message must be mutable (e.g., a non-nil pointer to a message).
+//
+// The error is io.EOF error only if no bytes are read.
+// If an EOF happens after reading some but not all the bytes,
+// UnmarshalNext returns a non-io.EOF error.
+func (d *Decoder) UnmarshalNext(m proto.Message) error {
+ size, err := binary.ReadUvarint(d.bufReader)
+ if err != nil {
+ return err
+ }
+
+ b := make([]byte, size)
+ if _, err := io.ReadFull(d.bufReader, b); err != nil {
+ return err
+ }
+ if err := d.opts.Unmarshal(b, m); err != nil {
+ return err
+ }
+ return nil
+}
diff --git a/encoding/protodelim/protodelim_test.go b/encoding/protodelim/protodelim_test.go
new file mode 100644
index 0000000..d0b6795
--- /dev/null
+++ b/encoding/protodelim/protodelim_test.go
@@ -0,0 +1,55 @@
+package protodelim_test
+
+import (
+ "bytes"
+ "errors"
+ "io"
+ "testing"
+
+ "github.com/google/go-cmp/cmp"
+ "google.golang.org/protobuf/encoding/protodelim"
+ "google.golang.org/protobuf/internal/testprotos/test3"
+ "google.golang.org/protobuf/testing/protocmp"
+)
+
+func TestRoundTrip(t *testing.T) {
+ msgs := []*test3.TestAllTypes{
+ {SingularInt32: 1},
+ {SingularString: "hello"},
+ {RepeatedDouble: []float64{1.2, 3.4}},
+ {
+ SingularNestedMessage: &test3.TestAllTypes_NestedMessage{A: 1},
+ RepeatedForeignMessage: []*test3.ForeignMessage{{C: 2}, {D: 3}},
+ },
+ }
+
+ buf := &bytes.Buffer{}
+
+ // Write all messages to buf.
+ for _, m := range msgs {
+ if n, err := protodelim.MarshalTo(buf, m); err != nil {
+ t.Errorf("protodelim.MarshalTo(_, %v) = %d, %v", m, n, err)
+ }
+ }
+
+ // Read and collect messages from buf.
+ decoder := protodelim.NewDecoder(buf)
+ var got []*test3.TestAllTypes
+ for {
+ m := &test3.TestAllTypes{}
+ err := decoder.UnmarshalNext(m)
+ if errors.Is(err, io.EOF) {
+ break
+ }
+ if err != nil {
+ t.Errorf("decoder.UnmarshalNext(_) = %v", err)
+ continue
+ }
+ got = append(got, m)
+ }
+
+ want := msgs
+ if diff := cmp.Diff(want, got, protocmp.Transform()); diff != "" {
+ t.Errorf("Decoder collected messages: diff -want +got: %s", diff)
+ }
+}
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Damien Neil, Lasse Folger, Sachin Padmanabhan.
1 comment:
Patchset:
Moving myself to CC. I no longer work at Google and protocol buffers. Adding the people would would be able to authoritatively say whether to accept new API for this.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Michael Stapelberg (Gerrit)
Attention is currently required from: Damien Neil, Lasse Folger, Sachin Padmanabhan.
Patch set 1:Code-Review +1
1 comment:
Patchset:
Thanks for your contribution! This looks reasonable to me, but I’d like dneil’s review as well in case there are some conventions regarding naming and API design that I’m not yet aware of.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Lasse Folger (Gerrit)
Attention is currently required from: Damien Neil, Sachin Padmanabhan.
1 comment:
Patchset:
Removing myself for now as Michael will review this CL.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Sachin Padmanabhan.
5 comments:
File encoding/protodelim/protodelim.go:
Patch Set #1, Line 23:
numSizeBytes := binary.PutUvarint(sizeBuf[:], uint64(len(msgBytes)))sizeBuf := protowire.AppendVarint(nil, uint64(len(msgBytes)))
Patch Set #1, Line 44:
// Decoder is used to parse a stream of varint size-delimited wire-format messages.Marshal and unmarshal should have a consistent API: Either a stateless MarshalOptions.WriteTo/UnmarshalOptions.ReadFrom as I proposed in golang/protobuf#1382, or a stateful Encoder/Decoder, but not one of each.
My preference is for the stateless design, which works better in scenarios where a size-delimited message is mixed in with other data in a stream.
Patch Set #1, Line 57:
return &Decoder{opts: o, bufReader: bufio.NewReader(r)}I think this package should take a *bufio.Reader.
bufio.NewReader(r) will return r if it's already a *bufio.Reader, but this only works if there are no intermediate layers wrapping the reader. Given that the protodelim package needs a buffered reader to avoid extreme inefficiency, making it explicit to the user seems like the best way to avoid confusion.
(See also https://github.com/golang/protobuf/issues/1382#issuecomment-992732953.)
Patch Set #1, Line 68:
size, err := binary.ReadUvarint(d.bufReader)This does work, and the protowire package doesn't provide a simple way to read from an io.Reader. However, the error returned by binary.ReadUvarint will be inconsistent with that returned by other functions in the protobuf module. I think this should use protowire for consistency.
I think something like this works:
b, err := d.bufReader.Peek(10)
if err != nil && err != io.EOF {
return err
}
size, n := protowire.ConsumeVarint(b)
if n < 0 {
return protowire.ParseError(n)
}
d.bufReader.Discard(n)Patch Set #1, Line 73:
b := make([]byte, size)There should be a way to set a maximum size to avoid arbitrarily large allocations here.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Sachin Padmanabhan.
Sachin Padmanabhan uploaded patch set #2 to this change.
encoding: add protodelim package
Inspired by discussion https://github.com/golang/protobuf/issues/1382
Change-Id: I30dc9bf9aa44e35cde8fb472c3b8b116d459714e
---
A encoding/protodelim/protodelim.go
A encoding/protodelim/protodelim_test.go
M encoding/protowire/wire.go
3 files changed, 205 insertions(+), 0 deletions(-)
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil.
5 comments:
File encoding/protodelim/protodelim.go:
Patch Set #1, Line 23:
numSizeBytes := binary.PutUvarint(sizeBuf[:], uint64(len(msgBytes)))
sizeBuf := protowire. […]
Done
Patch Set #1, Line 44:
// Decoder is used to parse a stream of varint size-delimited wire-format messages.
Marshal and unmarshal should have a consistent API: Either a stateless MarshalOptions. […]
Done
Patch Set #1, Line 57:
return &Decoder{opts: o, bufReader: bufio.NewReader(r)}
I think this package should take a *bufio.Reader. […]
Done
Patch Set #1, Line 68:
size, err := binary.ReadUvarint(d.bufReader)
This does work, and the protowire package doesn't provide a simple way to read from an io.Reader. […]
Done
Patch Set #1, Line 73:
b := make([]byte, size)There should be a way to set a maximum size to avoid arbitrarily large allocations here.
Done
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Sachin Padmanabhan.
Patch set 2:Run-TryBot +1
7 comments:
Patchset:
Looks pretty good to me with a few more nitpicks; thanks for the contribution!
Michael, I'd appreciate a second opinion on naming (MarshalTo or WriteTo?), the use of a concrete *bufio.Reader (I think this is the best choice, but there are tradeoffs here), and the default behavior when UnmarshalOptions.MaxSize==0.
File encoding/protodelim/protodelim.go:
Patch Set #2, Line 6:
"errors""google.golang.org/protobuf/internal/errors"
We use a module-internal errors package to get certain common behavior, such as ensuring errors created with `errors.New` use a common prefix and match the `proto.Error` error value.
Patch Set #2, Line 48:
MaxSize uint64Document the meaning of a zero MaxSize.
Also, a MaxSize of -1 should probably disable the limit.
Patch Set #2, Line 60:
UnmarhshalOptionstypo: UnmarshalOptions
Patch Set #2, Line 75:
func (o UnmarshalOptions) UnmarshalNext(r *bufio.Reader, m proto.Message) error {I think `UnmarshalFrom`. Joe's suggestion of `UnmarshalNext` was for a function which reads the next message from a `[]byte`, and I think we want to keep that name for that case if we ever implement it.
Patch Set #2, Line 93:
return &ErrSizeTooLarge{Size: size, MaxSize: maxSize}return errors.Wrap(&ErrSizeTooLarge{Size: size, MaxSize: maxSize}, "")
Every error returned by the protobuf module satisfies the condition `errors.Is(err, proto.Error)`, and has a common "proto: " prefix. errors.Wrap will ensure these properties are satisfied.
Returning a error which wraps *ErrSizeTooLarge rather than *ErrSizeTooLarge itself also has the nice property that it ensures anyone checking for this error will be using errors.As, not a direct type assertion.Patch Set #2, Line 97:
if _, err := io.ReadFull(r, b); err != nil {I don't think we have any existing functions in the protobuf module which operate on an io.Reader/io.Writer, so we haven't had to answer the question of whether we pass through IO errors unwrapped or add an extra layer of protobuf-module wrapping.
I think that what you have here is fine: If a read or write fails, we just return the error without any wrapping. The MarshalTo/UnmarshalFrom function docs should probably mention this, though:
// If the Writer returns an error, MarshalTo returns it unchanged.
// If the Reader returns an error other than io.EOF, UnmarshalFrom returns it unchanged.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Michael Stapelberg (Gerrit)
Attention is currently required from: Damien Neil, Sachin Padmanabhan.
Patch set 2:Code-Review +1
2 comments:
Patchset:
Michael, I'd appreciate a second opinion on naming (MarshalTo or WriteTo?),
I think we should stick to existing terminology of the proto package as much as possible, so MarshalTo sounds better to me compared to WriteTo. Unless I’m missing a nuance here…?
the use of a concrete *bufio.Reader (I think this is the best choice, but there are tradeoffs here),
Using a concrete *bufio.Reader LGTM.
and the default behavior when UnmarshalOptions.MaxSize==0.
On the one hand, I personally seem to always run into such limits (e.g. the gRPC max message size) :)
On the other hand, I think defaulting to any limit is preferable over defaulting to no limit, from a safety perspective.
Are you asking whether the specific default limit seems reasonable, or whether limiting by default at all seems reasonable?
Thank you for reviewing this, Damien!
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Michael Stapelberg, Sachin Padmanabhan.
1 comment:
Patchset:
Are you asking whether the specific default limit seems reasonable, or whether limiting by default at all seems reasonable?
Yes. :)
The current behavior here seems pretty reasonable to me, but I can see an argument for off-by-default.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
Sachin Padmanabhan uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Damien Neil
encoding: add protodelim package
Inspired by discussion https://github.com/golang/protobuf/issues/1382
Change-Id: I30dc9bf9aa44e35cde8fb472c3b8b116d459714e
---
A encoding/protodelim/protodelim.go
A encoding/protodelim/protodelim_test.go
M encoding/protowire/wire.go
3 files changed, 211 insertions(+), 0 deletions(-)
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg.
7 comments:
Patchset:
Thanks so much for the review!
File encoding/protodelim/protodelim.go:
Patch Set #2, Line 6:
"errors"
Patch Set #2, Line 48:
MaxSize uint64
Document the meaning of a zero MaxSize. […]
Done
Patch Set #2, Line 60:
UnmarhshalOptionstypo: UnmarshalOptions
Done
Patch Set #2, Line 75:
func (o UnmarshalOptions) UnmarshalNext(r *bufio.Reader, m proto.Message) error {
I think `UnmarshalFrom`. […]
Done
Patch Set #2, Line 93:
return &ErrSizeTooLarge{Size: size, MaxSize: maxSize}
return errors.Wrap(&ErrSizeTooLarge{Size: size, MaxSize: maxSize}, "") […]
Done
Patch Set #2, Line 97:
if _, err := io.ReadFull(r, b); err != nil {
I don't think we have any existing functions in the protobuf module which operate on an io. […]
Done
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg.
5 comments:
Patchset:
> Are you asking whether the specific default limit seems reasonable, or whether limiting by default […]
I'm fairly saddened by the concrete use of *bufio.Reader. I feel like it should at least be an interface to provide future flexibility.
// PeekableReader is a reader that can be peeked into.
// It is implemented by bufio.Reader.
type PeekableReader interface {
// Peek returns the next n bytes without advancing the reader.
// The bytes stop being valid at the next read call.
// If Peek returns fewer than n bytes,
// it also returns an error explaining why the read is short.
Peek(n int) ([]byte, error)io.Reader
}We don't need discard since we can just allocate a few extra bytes when craft a `[]byte` for the message payload.
File encoding/protodelim/protodelim.go:
Patch Set #2, Line 51:
41943044 << 20 is probably more readable.
Should this be exported?
Patch Set #2, Line 55:
type ErrSizeTooLarge struct {Shouldn't this be SizeTooLargeError or something?
Type names usually have `Error` as a suffix (e.g., net.ParseError).
Var names usually have `Err` as a prefix (e.g., io.ErrClosedPipe).Patch Set #2, Line 98:
return errAdditionally check:
if err == io.EOF {
err = io.ErrUnexpectedEOF
}and add a test for this behavior.
There's a documented gotcha in io.ReadFull where:
The error is EOF only if no bytes were read.
Since we previously read a length-prefix, we should never be returning io.EOF afterwards since we should expect a message.
File encoding/protowire/wire.go:
Patch Set #2, Line 71:
const MaxVarintLen = 10Perhaps MaxVarintSize since the code below uses the term "Size" instead of "Len" in the declared funcs (e.g., SizeVarint).
The odd ordering of names is probably my fault, but perhaps name it `SizeVarintMax` to be consistent?
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
1 comment:
File encoding/protowire/wire.go:
Patch Set #2, Line 71:
const MaxVarintLen = 10
Perhaps MaxVarintSize since the code below uses the term "Size" instead of "Len" in the declared fun […]
Actually, I think we can just avoid this constant and call `protowire.SizeVarint(math.MaxUint64)` in your code. The `SizeVarint` function is inlineable, so it will be reduced down to a constant by the compiler anyways.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Michael Stapelberg, Sachin Padmanabhan.
2 comments:
Patchset:
Sorry, one last nitpick caught by running test.bash.
File encoding/protodelim/protodelim.go:
Patch Set #3, Line 1:
// Package protodelim marshals and unmarshals varint size-delimited messages.Needs standard copyright header:
// Copyright 2022 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
(And protodelim_test.go as well.)
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Michael Stapelberg, Sachin Padmanabhan.
3 comments:
File encoding/protodelim/protodelim.go:
Patch Set #2, Line 1:
// Package protodelim marshals and unmarshals varint size-delimited messages.Is a LICENSE header needed here? (Also in the test file).
File encoding/protodelim/protodelim_test.go:
Patch Set #2, Line 16:
func TestProtodelim_RoundTrip(t *testing.T) {Just call it TestRoundTrip. The "Protodelim" part is redundant with the fact that this is in the protodelim test package.
Patch Set #2, Line 58:
TestProtodelim_MaxSizeJust call it TestMaxSize. The "Protodelim" part is redundant with the fact that this is in the protodelim test package.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Michael Stapelberg, Sachin Padmanabhan.
1 comment:
File encoding/protodelim/protodelim.go:
Patch Set #3, Line 83:
!errors.Is(err, io.EOF)By convention, `io.EOF` is never wrapped. This should just be `err != io.EOF`.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Joseph Tsai, Michael Stapelberg, Sachin Padmanabhan.
1 comment:
Patchset:
I'm fairly saddened by the concrete use of *bufio.Reader. […]
Perhaps io.ByteReader?
Russ points out that flate.NewReader uses ByteReader (via type assertion) and that it doesn't seem to have led to performance complaints.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Joseph Tsai, Michael Stapelberg, Sachin Padmanabhan.
Sachin Padmanabhan uploaded patch set #4 to this change.
encoding: add protodelim package
Inspired by discussion https://github.com/golang/protobuf/issues/1382
Change-Id: I30dc9bf9aa44e35cde8fb472c3b8b116d459714e
---
A encoding/protodelim/protodelim.go
A encoding/protodelim/protodelim_test.go
2 files changed, 238 insertions(+), 0 deletions(-)
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Michael Stapelberg.
10 comments:
Patchset:
I'm fairly saddened by the concrete use of *bufio.Reader. […]
I don't have a strong opinion on this, will leave it to you and dneil
File encoding/protodelim/protodelim.go:
Patch Set #2, Line 1:
// Package protodelim marshals and unmarshals varint size-delimited messages.Is a LICENSE header needed here? (Also in the test file).
Done
Patch Set #2, Line 51:
4194304
4 << 20 is probably more readable. […]
Done
Up to you on whether it is exported.
Patch Set #2, Line 55:
type ErrSizeTooLarge struct {
Shouldn't this be SizeTooLargeError or something? […]
Done
Great point!
Patch Set #2, Line 98:
return err
Additionally check: […]
Done
Great catch
File encoding/protodelim/protodelim.go:
Patch Set #3, Line 1:
// Package protodelim marshals and unmarshals varint size-delimited messages.Needs standard copyright header: […]
Done
Patch Set #3, Line 83:
!errors.Is(err, io.EOF)By convention, `io.EOF` is never wrapped. This should just be `err != io.EOF`.
Done
File encoding/protodelim/protodelim_test.go:
Patch Set #2, Line 16:
func TestProtodelim_RoundTrip(t *testing.T) {
Just call it TestRoundTrip. […]
Done
Patch Set #2, Line 58:
TestProtodelim_MaxSize
Just call it TestMaxSize. […]
Done
File encoding/protowire/wire.go:
Patch Set #2, Line 71:
const MaxVarintLen = 10
Actually, I think we can just avoid this constant and call `protowire.SizeVarint(math. […]
Done
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
1 comment:
Patchset:
I don't have a strong opinion on this, will leave it to you and dneil
I'd be okay with using what is effectively the `flate.ByteReader` interface, but declaring a local version of the interface. In the common case, we'd only be calling the `ReadByte` method 2-4 times per message, which isn't a problem. Also the cost of calling `ReadByte` is almost certainly going to be overshadowed by the cost o unmarshaling the message itself.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
2 comments:
File encoding/protodelim/protodelim.go:
This should be a blank line and not a "//" to split the copyright header from the Go package docs.
File encoding/protodelim/protodelim_test.go:
Patch Set #4, Line 4:
package protodelim_testBlank line before package name, otherwise the copyright message is treated as the package docs.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
Sachin Padmanabhan uploaded patch set #5 to this change.
encoding: add protodelim package
Inspired by discussion https://github.com/golang/protobuf/issues/1382
Change-Id: I30dc9bf9aa44e35cde8fb472c3b8b116d459714e
---
A encoding/protodelim/protodelim.go
A encoding/protodelim/protodelim_test.go
2 files changed, 239 insertions(+), 0 deletions(-)
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Michael Stapelberg.
2 comments:
File encoding/protodelim/protodelim.go:
This should be a blank line and not a "//" to split the copyright header from the Go package docs.
Done
File encoding/protodelim/protodelim_test.go:
Patch Set #4, Line 4:
package protodelim_testBlank line before package name, otherwise the copyright message is treated as the package docs.
Done
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Michael Stapelberg.
1 comment:
Patchset:
Hi guys, just wanted to bump this -- have we come to a decision on taking *bufio.Reader?
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
1 comment:
Patchset:
Hi guys, just wanted to bump this -- have we come to a decision on taking *bufio. […]
My vote is for a flate.Reader-like interface, but I defer to @dneil's decision.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Joseph Tsai, Michael Stapelberg, Sachin Padmanabhan.
1 comment:
Patchset:
My vote is for a flate.Reader-like interface, but I defer to @dneil's decision.
Sure, let's match flate.Reader:
type Reader interface {
io.Reader
io.ByteReader
}
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Michael Stapelberg.
1 comment:
Patchset:
Sure, let's match flate.Reader: […]
Is the idea to always ReadByte 10 bytes a la Peek? Or just read one-by-one and try protowire.ConsumeVarint after each until it works?
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
1 comment:
Patchset:
Is the idea to always ReadByte 10 bytes a la Peek? Or just read one-by-one and try protowire. […]
You can do:
var a [binary.MaxVarintLen64]byte
b := a[:0]
for i := range arr {
c, err := r.ReadByte()
if err != nil && (err != io.EOF || i == 0) {
return err
}
b = append(b, c)
if c < 0x80 {
break
}
}
size, n := protowire.ConsumeVarint(b)This is more complicated than simplying doing:
n, err := binary.ReadUvarint(r)
but ensures that we have consistent errors produced by protowire.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
1 comment:
Patchset:
You can do: […]
Not to mention, there's in binary.Uvarint. Yay.
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
1 comment:
Patchset:
Not to mention, there's in binary.Uvarint. Yay. […]
Meant to say "there's a bug in binary.Uvarint"
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
Sachin Padmanabhan uploaded patch set #6 to this change.
encoding: add protodelim package
Inspired by discussion https://github.com/golang/protobuf/issues/1382
Change-Id: I30dc9bf9aa44e35cde8fb472c3b8b116d459714e
---
A encoding/protodelim/protodelim.go
A encoding/protodelim/protodelim_test.go
2 files changed, 249 insertions(+), 0 deletions(-)
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Michael Stapelberg.
2 comments:
Patchset:
I'd be okay with using what is effectively the `flate. […]
Done
Patchset:
Meant to say "there's a bug in binary. […]
Ah, makes sense.
Done
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
1 comment:
File encoding/protodelim/protodelim.go:
Patch Set #6, Line 94:
sizeBuf := make([]byte, protowire.SizeVarint(math.MaxUint64))I highly suspect this will heap allocate since the compiler is not smart enough to realize that this is a constant, fixed-sized allocation.
Also, we need to track the number of bytes read otherwise ConsumeVarint will not properly detect io.ErrUnexpectedEOF.
Thus:
var sizeArr [binary.MaxVarintLen64]byte
sizeBuf := sizeArr[:0]
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
Sachin Padmanabhan uploaded patch set #7 to this change.
encoding: add protodelim package
Inspired by discussion https://github.com/golang/protobuf/issues/1382
Change-Id: I30dc9bf9aa44e35cde8fb472c3b8b116d459714e
---
A encoding/protodelim/protodelim.go
A encoding/protodelim/protodelim_test.go
2 files changed, 250 insertions(+), 0 deletions(-)
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Joseph Tsai, Michael Stapelberg.
1 comment:
File encoding/protodelim/protodelim.go:
Patch Set #6, Line 94:
sizeBuf := make([]byte, protowire.SizeVarint(math.MaxUint64))
I highly suspect this will heap allocate since the compiler is not smart enough to realize that this […]
Ah, I see, totally escaped me that the argument to ConsumeVarint needs to be just the varint bytes
Done
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Joseph Tsai (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg, Sachin Padmanabhan.
Patch set 7:Code-Review +2
Michael Stapelberg (Gerrit)
Attention is currently required from: Damien Neil, Sachin Padmanabhan.
Patch set 7:Code-Review +2
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Attention is currently required from: Sachin Padmanabhan.
Patch set 7:Code-Review +2
2 comments:
Commit Message:
Patch Set #7, Line 9:
Inspired by discussion https://github.com/golang/protobuf/issues/1382One last nitpick:
Fixes golang/protobuf#1382
This specific syntax lets gopherbot associate the CL and the issue.
Patchset:
Thanks again for the change, and for bearing with the revisions!
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Sachin Padmanabhan.
Sachin Padmanabhan uploaded patch set #8 to this change.
encoding: add protodelim package
Fixes golang/protobuf#1382
Change-Id: I30dc9bf9aa44e35cde8fb472c3b8b116d459714e
---
A encoding/protodelim/protodelim.go
A encoding/protodelim/protodelim_test.go
2 files changed, 250 insertions(+), 0 deletions(-)
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
2 comments:
Commit Message:
Patch Set #7, Line 9:
Inspired by discussion https://github.com/golang/protobuf/issues/1382
One last nitpick: […]
Done
Patchset:
Thanks everyone for the reviews!
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Sachin Padmanabhan (Gerrit)
Attention is currently required from: Damien Neil, Michael Stapelberg.
1 comment:
Patchset:
Adding dneil and stapelberg back to attention set for submission
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.
Damien Neil (Gerrit)
Damien Neil submitted this change.
7 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
encoding: add protodelim package
Fixes golang/protobuf#1382
Change-Id: I30dc9bf9aa44e35cde8fb472c3b8b116d459714e
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/419254
Reviewed-by: Michael Stapelberg <stape...@google.com>
Reviewed-by: Damien Neil <dn...@google.com>
Reviewed-by: Joseph Tsai <joe...@digital-static.net>
---
A encoding/protodelim/protodelim.go
A encoding/protodelim/protodelim_test.go
2 files changed, 254 insertions(+), 0 deletions(-)
diff --git a/encoding/protodelim/protodelim.go b/encoding/protodelim/protodelim.go
new file mode 100644
index 0000000..e2b6cd4
--- /dev/null
+++ b/encoding/protodelim/protodelim.go
@@ -0,0 +1,140 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package protodelim marshals and unmarshals varint size-delimited messages.
+package protodelim
+
+import (
+ "encoding/binary"
+ "fmt"
+ "io"
+
+ "google.golang.org/protobuf/encoding/protowire"
+ "google.golang.org/protobuf/internal/errors"
+ "google.golang.org/protobuf/proto"
+)
+
+// MarshalOptions is a configurable varint size-delimited marshaler.
+type MarshalOptions struct{ proto.MarshalOptions }
+
+// MarshalTo writes a varint size-delimited wire-format message to w.
+// If w returns an error, MarshalTo returns it unchanged.
+func (o MarshalOptions) MarshalTo(w io.Writer, m proto.Message) (int, error) {
+ msgBytes, err := o.MarshalOptions.Marshal(m)
+ if err != nil {
+ return 0, err
+ }
+
+ sizeBytes := protowire.AppendVarint(nil, uint64(len(msgBytes)))
+ sizeWritten, err := w.Write(sizeBytes)
+ if err != nil {
+ return sizeWritten, err
+ }
+ msgWritten, err := w.Write(msgBytes)
+ if err != nil {
+ return sizeWritten + msgWritten, err
+ }
+ return sizeWritten + msgWritten, nil
+}
+
+// MarshalTo writes a varint size-delimited wire-format message to w
+// with the default options.
+//
+// See the documentation for MarshalOptions.MarshalTo.
+func MarshalTo(w io.Writer, m proto.Message) (int, error) {
+ return MarshalOptions{}.MarshalTo(w, m)
+}
+
+// UnmarshalOptions is a configurable varint size-delimited unmarshaler.
+type UnmarshalOptions struct {
+ proto.UnmarshalOptions
+
+ // MaxSize is the maximum size in wire-format bytes of a single message.
+ // Unmarshaling a message larger than MaxSize will return an error.
+ // A zero MaxSize will default to 4 MiB.
+ // Setting MaxSize to -1 disables the limit.
+ MaxSize int64
+}
+
+const defaultMaxSize = 4 << 20 // 4 MiB, corresponds to the default gRPC max request/response size
+
+// SizeTooLargeError is an error that is returned when the unmarshaler encounters a message size
+// that is larger than its configured MaxSize.
+type SizeTooLargeError struct {
+ // Size is the varint size of the message encountered
+ // that was larger than the provided MaxSize.
+ Size uint64
+
+ // MaxSize is the MaxSize limit configured in UnmarshalOptions, which Size exceeded.
+ MaxSize uint64
+}
+
+func (e *SizeTooLargeError) Error() string {
+ return fmt.Sprintf("message size %d exceeded unmarshaler's maximum configured size %d", e.Size, e.MaxSize)
+}
+
+// Reader is the interface expected by UnmarshalFrom.
+// It is implemented by *bufio.Reader.
+type Reader interface {
+ io.Reader
+ io.ByteReader
+}
+
+// UnmarshalFrom parses and consumes a varint size-delimited wire-format message
+// from r.
+// The provided message must be mutable (e.g., a non-nil pointer to a message).
+//
+// The error is io.EOF error only if no bytes are read.
+// If an EOF happens after reading some but not all the bytes,
+// UnmarshalFrom returns a non-io.EOF error.
+// In particular if r returns a non-io.EOF error, UnmarshalFrom returns it unchanged,
+// and if only a size is read with no subsequent message, io.ErrUnexpectedEOF is returned.
+func (o UnmarshalOptions) UnmarshalFrom(r Reader, m proto.Message) error {
+ var sizeArr [binary.MaxVarintLen64]byte
+ sizeBuf := sizeArr[:0]
+ for i := range sizeArr {
+ b, err := r.ReadByte()
+ if err != nil && (err != io.EOF || i == 0) {
+ return err
+ }
+ sizeBuf = append(sizeBuf, b)
+ if b < 0x80 {
+ break
+ }
+ }
+ size, n := protowire.ConsumeVarint(sizeBuf)
+ if n < 0 {
+ return protowire.ParseError(n)
+ }
+
+ maxSize := o.MaxSize
+ if maxSize == 0 {
+ maxSize = defaultMaxSize
+ }
+ if maxSize != -1 && size > uint64(maxSize) {
+ return errors.Wrap(&SizeTooLargeError{Size: size, MaxSize: uint64(maxSize)}, "")
+ }
+
+ b := make([]byte, size)
+ _, err := io.ReadFull(r, b)
+ if err == io.EOF {
+ return io.ErrUnexpectedEOF
+ }
+ if err != nil {
+ return err
+ }
+ if err := o.Unmarshal(b, m); err != nil {
+ return err
+ }
+ return nil
+}
+
+// UnmarshalFrom parses and consumes a varint size-delimited wire-format message
+// from r with the default options.
+// The provided message must be mutable (e.g., a non-nil pointer to a message).
+//
+// See the documentation for UnmarshalOptions.UnmarshalFrom.
+func UnmarshalFrom(r Reader, m proto.Message) error {
+ return UnmarshalOptions{}.UnmarshalFrom(r, m)
+}
diff --git a/encoding/protodelim/protodelim_test.go b/encoding/protodelim/protodelim_test.go
new file mode 100644
index 0000000..9c2458b
--- /dev/null
+++ b/encoding/protodelim/protodelim_test.go
@@ -0,0 +1,99 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package protodelim_test
+
+import (
+ "bufio"
+ "bytes"
+ "errors"
+ "io"
+ "testing"
+
+ "github.com/google/go-cmp/cmp"
+ "google.golang.org/protobuf/encoding/protodelim"
+ "google.golang.org/protobuf/encoding/protowire"
+ "google.golang.org/protobuf/internal/testprotos/test3"
+ "google.golang.org/protobuf/testing/protocmp"
+)
+
+func TestRoundTrip(t *testing.T) {
+ msgs := []*test3.TestAllTypes{
+ {SingularInt32: 1},
+ {SingularString: "hello"},
+ {RepeatedDouble: []float64{1.2, 3.4}},
+ {
+ SingularNestedMessage: &test3.TestAllTypes_NestedMessage{A: 1},
+ RepeatedForeignMessage: []*test3.ForeignMessage{{C: 2}, {D: 3}},
+ },
+ }
+
+ buf := &bytes.Buffer{}
+
+ // Write all messages to buf.
+ for _, m := range msgs {
+ if n, err := protodelim.MarshalTo(buf, m); err != nil {
+ t.Errorf("protodelim.MarshalTo(_, %v) = %d, %v", m, n, err)
+ }
+ }
+
+ // Read and collect messages from buf.
+ var got []*test3.TestAllTypes
+ r := bufio.NewReader(buf)
+ for {
+ m := &test3.TestAllTypes{}
+ err := protodelim.UnmarshalFrom(r, m)
+ if errors.Is(err, io.EOF) {
+ break
+ }
+ if err != nil {
+ t.Errorf("protodelim.UnmarshalFrom(_) = %v", err)
+ continue
+ }
+ got = append(got, m)
+ }
+
+ want := msgs
+ if diff := cmp.Diff(want, got, protocmp.Transform()); diff != "" {
+ t.Errorf("Unmarshaler collected messages: diff -want +got = %s", diff)
+ }
+}
+
+func TestMaxSize(t *testing.T) {
+ in := &test3.TestAllTypes{SingularInt32: 1}
+
+ buf := &bytes.Buffer{}
+
+ if n, err := protodelim.MarshalTo(buf, in); err != nil {
+ t.Errorf("protodelim.MarshalTo(_, %v) = %d, %v", in, n, err)
+ }
+
+ out := &test3.TestAllTypes{}
+ err := protodelim.UnmarshalOptions{MaxSize: 1}.UnmarshalFrom(bufio.NewReader(buf), out)
+
+ var errSize *protodelim.SizeTooLargeError
+ if !errors.As(err, &errSize) {
+ t.Errorf("protodelim.UnmarshalOptions{MaxSize: 1}.UnmarshalFrom(_, _) = %v (%T), want %T", err, err, errSize)
+ }
+ got, want := errSize, &protodelim.SizeTooLargeError{Size: 3, MaxSize: 1}
+ if diff := cmp.Diff(want, got); diff != "" {
+ t.Errorf("protodelim.UnmarshalOptions{MaxSize: 1}.UnmarshalFrom(_, _): diff -want +got = %s", diff)
+ }
+}
+
+func TestUnmarshalFrom_UnexpectedEOF(t *testing.T) {
+ buf := &bytes.Buffer{}
+
+ // Write a size (42), but no subsequent message.
+ sb := protowire.AppendVarint(nil, 42)
+ if _, err := buf.Write(sb); err != nil {
+ t.Fatalf("buf.Write(%v) = _, %v", sb, err)
+ }
+
+ out := &test3.TestAllTypes{}
+ err := protodelim.UnmarshalFrom(bufio.NewReader(buf), out)
+ if got, want := err, io.ErrUnexpectedEOF; got != want {
+ t.Errorf("protodelim.UnmarshalFrom(size-only buf, _) = %v, want %v", got, want)
+ }
+}
To view, visit change 419254. To unsubscribe, or for help writing mail filters, visit settings.