Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Return EOF and ErrUnexpectedEOF correctly #64

Merged
merged 16 commits into from
Mar 17, 2022
Merged

Return EOF and ErrUnexpectedEOF correctly #64

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
099ab3e
Use readerWithEOFContext to handle EOF vs ErrUnexpectedEOF cases
MarcoPolo Mar 9, 2022
6eb0095
fix: specialize discard for readerWithEOFContext
Stebalien Mar 10, 2022
0326313
Check hasReadOnce in readByte methods
MarcoPolo Mar 10, 2022
eecee09
Avoid allocation in ScanForLinks
MarcoPolo Mar 10, 2022
1fdee4f
fix: export eof reader and fix compile
Stebalien Mar 10, 2022
3dcc3ab
feat: avoid creating new ReaderWithEOFContexts where possible
Stebalien Mar 10, 2022
4343a5f
Fix GetPeeker magic
MarcoPolo Mar 10, 2022
22488b2
Use defer and remove ReaderWithEOFContext
MarcoPolo Mar 10, 2022
083da62
feat: simplify "read once" checks
Stebalien Mar 10, 2022
86717ea
fix: check for unexpected EOF in deferred unmarshaling
Stebalien Mar 10, 2022
e120cdc
Add TestErrUnexpectedEOF
MarcoPolo Mar 10, 2022
ae38a94
Use errors.Is to detect wrapped EOF errors
MarcoPolo Mar 10, 2022
b23fed4
Wrap error at end so we don't lose error context
MarcoPolo Mar 10, 2022
19357f6
Remove expectation of ErrUnexpectedEOF we just want to make sure we d…
MarcoPolo Mar 10, 2022
0396fd1
Revert "Wrap error at end so we don't lose error context"
MarcoPolo Mar 10, 2022
c353e0a
Revert "Use errors.Is to detect wrapped EOF errors"
MarcoPolo Mar 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions gen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import (
"io"
"math"
"sort"
"errors"

{{ range .Imports }}{{ .Name }} "{{ .PkgPath }}"
{{ end }}
Expand Down Expand Up @@ -1032,7 +1033,7 @@ func emitCborUnmarshalSliceField(w io.Writer, f Field) error {

func emitCborUnmarshalStructTuple(w io.Writer, gti *GenTypeInfo) error {
err := doTemplate(w, gti, `
func (t *{{ .Name}}) UnmarshalCBOR(r io.Reader) error {
func (t *{{ .Name}}) UnmarshalCBOR(r io.Reader) (err error) {
*t = {{.Name}}{}

br := cbg.GetPeeker(r)
Expand All @@ -1042,6 +1043,12 @@ func (t *{{ .Name}}) UnmarshalCBOR(r io.Reader) error {
if err != nil {
return err
}
defer func() {
if errors.Is(err, io.EOF) {
err = xerrors.Errorf("%w: %v", io.ErrUnexpectedEOF, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't quite work. %w needs to be last.

It's probably fine to just leave it as-is? The only thing we really care about is not returning an EOF if we've read bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it? https://go.dev/play/p/k2wOyoCk4WO makes it seem like both work.

I figured we cared enough to wrap the original error, so might be nice to keep that info.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, being last only matters for Unwrap. (docs: https://pkg.go.dev/golang.org/x/xerrors#Errorf)

I figured we cared enough to wrap the original error, so might be nice to keep that info.

My point was: that we could just leave everything as-is. I.e., we don't want an EOF unless we read no data, but we're fine with any other error (including a wrapped EOF) otherwise.

But if this works and doesn't hurt performance too much, then it's fine by me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, being last only matters for Unwrap. (docs: https://pkg.go.dev/golang.org/x/xerrors#Errorf)

From that link

If the format specifier includes a %w verb with an error operand in a position other than at the end, the returned error will still implement an Unwrap method returning the operand, but the error's Format method will not return the wrapped error.

What is the "error's Format method" ?

anyways, I understand what you mean by as-is. I changed this to return the wrapped error and only replace if we return EOF. I also changed the test to only check that we aren't returning EOF exactly if it could read some bytes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the "error's Format method" ?

🤷‍♂️ I'm having a lot of difficulty with those docs.

}
}()

if maj != cbg.MajArray {
return fmt.Errorf("cbor input should be of type array")
}
Expand Down Expand Up @@ -1190,7 +1197,7 @@ func emitCborMarshalStructMap(w io.Writer, gti *GenTypeInfo) error {

func emitCborUnmarshalStructMap(w io.Writer, gti *GenTypeInfo) error {
err := doTemplate(w, gti, `
func (t *{{ .Name}}) UnmarshalCBOR(r io.Reader) error {
func (t *{{ .Name}}) UnmarshalCBOR(r io.Reader) (err error) {
*t = {{.Name}}{}

br := cbg.GetPeeker(r)
Expand All @@ -1200,6 +1207,12 @@ func (t *{{ .Name}}) UnmarshalCBOR(r io.Reader) error {
if err != nil {
return err
}
defer func() {
if errors.Is(err, io.EOF) {
err = xerrors.Errorf("%w: %v", io.ErrUnexpectedEOF, err)
}
}()

if maj != cbg.MajMap {
return fmt.Errorf("cbor input should be of type map")
}
Expand Down
49 changes: 43 additions & 6 deletions testing/cbor_gen.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 36 additions & 5 deletions testing/cbor_map_gen.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

53 changes: 44 additions & 9 deletions testing/roundtrip_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,16 @@ package testing
import (
"bytes"
"encoding/json"
"github.com/ipfs/go-cid"
"errors"
"io"
"math/rand"
"reflect"
"testing"
"testing/quick"
"time"

"github.com/ipfs/go-cid"

"github.com/google/go-cmp/cmp"
cbg "github.com/whyrusleeping/cbor-gen"
)
Expand Down Expand Up @@ -174,12 +177,12 @@ func TestLessToMoreFieldsRoundTrip(t *testing.T) {
NString: "namedstr",
}
obj := &SimpleStructV1{
OldStr: "hello",
OldBytes: []byte("bytes"),
OldNum: 10,
OldPtr: &dummyCid,
OldMap: map[string]SimpleTypeOne{"first": simpleTypeOne},
OldArray: []SimpleTypeOne{simpleTypeOne},
OldStr: "hello",
OldBytes: []byte("bytes"),
OldNum: 10,
OldPtr: &dummyCid,
OldMap: map[string]SimpleTypeOne{"first": simpleTypeOne},
OldArray: []SimpleTypeOne{simpleTypeOne},
OldStruct: simpleTypeOne,
}

Expand Down Expand Up @@ -274,8 +277,8 @@ func TestMoreToLessFieldsRoundTrip(t *testing.T) {
NewPtr: &dummyCid2,
OldMap: map[string]SimpleTypeOne{"foo": simpleType1},
NewMap: map[string]SimpleTypeOne{"bar": simpleType2},
OldArray: []SimpleTypeOne{simpleType1},
NewArray: []SimpleTypeOne{simpleType1, simpleType2},
OldArray: []SimpleTypeOne{simpleType1},
NewArray: []SimpleTypeOne{simpleType1, simpleType2},
OldStruct: simpleType1,
NewStruct: simpleType2,
}
Expand Down Expand Up @@ -315,3 +318,35 @@ func TestMoreToLessFieldsRoundTrip(t *testing.T) {
t.Fatal("mismatch struct marshal / unmarshal")
}
}

func TestErrUnexpectedEOF(t *testing.T) {
err := quick.Check(func(val SimpleTypeTwo, endIdx uint) bool {
return t.Run("quickcheck", func(t *testing.T) {
buf := new(bytes.Buffer)
if err := val.MarshalCBOR(buf); err != nil {
t.Error(err)
}

enc := buf.Bytes()
originalLen := len(enc)
endIdx = endIdx % uint(len(enc))
enc = enc[:endIdx]

nobj := SimpleTypeTwo{}
err := nobj.UnmarshalCBOR(bytes.NewReader(enc))
if int(endIdx) == originalLen && err != nil {
t.Fatal("failed to round trip object: ", err)
} else if endIdx == 0 && !errors.Is(err, io.EOF) {
t.Fatal("expected EOF got", err)
} else if endIdx != 0 && !errors.Is(err, io.ErrUnexpectedEOF) {
t.Fatal("expected ErrUnexpectedEOF got", endIdx, originalLen, err)
}
})

}, &quick.Config{MaxCount: 1000})

if err != nil {
t.Error(err)
}

}
Loading