Go: encoding/json: eof error of NewDecoder().Decode() should be same with Unmarshal()

[isayme]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.10.1 darwin/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64" GOOS="darwin"

What did you do?

https://play.golang.org/p/gr5cHhrpmbK

What did you expect to see?

shown in previous link

What did you see instead?

shown in previous link

[agnivade]

/cc @dsnet

[isayme]

BTW. a solution is change io.ErrUnexpectedEOF to &SyntaxError{"unexpected end of JSON input", dec.scan.bytes} in https://github.com/golang/go/blob/c99300229de4e69220790c71da14785dc52c3d68/src/encoding/json/stream.go#L126

[ianlancetaylor]

Seems worth changing, want to send a CL?

[agnivade]

@ianlancetaylor - Just concerned, can it be considered a borderline breaking change ?

[ianlancetaylor]

@agnivade Yes, it's clearly possible to write a program that will observe this change. I'm guessing that few programs test explicitly for io.ErrUnexpectedEOF. But I don't actually know. The only way to find out would be to try it.

Actually thinking about it a bit more I'm not sure which way to change it. Is it better to always return a json.SyntaxError? Or is is better to not wrap io.ErrUnexpectedEOF in a json.SyntaxError, just as we generally avoid wrapping io.EOF?

[agnivade]

A quick github search for TestInvalidJSON gives some hits which are potential breakages.

• https://github.com/lemonldap-ng-controller/lemonldap-ng-controller/blob/a151ed71970d6199a936f028a25fcc2969da7aff/internal/lemonldapng/converter/converter_test.go#L56

• https://github.com/smagch/patch/blob/fee8a6dae4f796e0714a27c0a06d356e3059ce33/parse_test.go#L85

There might be more but I stopped searching after I got these.

Both of these do json.Unmarshal, so I guess it might be safer to change json.NewDecoder. But again, there just might be some code out there which breaks.

[dsnet]

Arguments for changing it to *json.SyntaxError:

• io.ErrUnexpectedEOF is documented as indicating that "EOF was encountered in the middle of reading a fixed-size block or data structure", for which this does not seem to be the case.
• This situation really does look like syntax error. There is no amount of characters that you can add to {"name":"} to make it valid JSON. EDIT: not true, you can append "} as well here.

That being said, I may have expected {"name":" to return io.ErrUnexpectedEOF, since it is possible to append "} and obtain valid JSON. However, this is not the case today, either.

See https://play.golang.org/p/93ozJzoQSRR

[dsnet]

I support changing it to io.ErrUnexpectedEOF for the following reasons:

• Despite the documentation on ErrUnexpectedEOF, I (and my observation of other parser code) has commonly interpreted that error as indicating that is some number of bytes you could be appended to the string to turn in into a valid input.
• It is generally safer to move from a less distinguishable to a more distinguishable error. While SyntaxError is distinguishable, it is functionally less so than ErrUnexpectedEOF, which only requires a comparison instead of a type assertion.

[agnivade]

@dsnet - I tried changing it to io.ErrUnexpectedEOF and several tests failed -.

--- FAIL: TestUnmarshal (0.00s)
    decode_test.go:1016: #38: checkValid: &errors.errorString{s:"unexpected EOF"}
--- FAIL: TestUnmarshalSyntax (0.00s)
    decode_test.go:1962: expected syntax error for Unmarshal("\"hello"): got *errors.errorString
    decode_test.go:1962: expected syntax error for Unmarshal("[1,2,3"): got *errors.errorString
    decode_test.go:1962: expected syntax error for Unmarshal("{\"key\":1"): got *errors.errorString
    decode_test.go:1962: expected syntax error for Unmarshal("{\"key\":1,"): got *errors.errorString
FAIL
FAIL    encoding/json   1.086s

Whereas, if I do the other way and change to SyntaxError, no tests fail. It seems to me that changing to SyntaxError may actually be less invasive.

But your call. Let me know.

[agnivade]

ping @dsnet @mvdan

[mvdan]

I know very little about encoding/json's history with errors, sorry. It sounds to me like parsing "hello is a SyntaxError by definition, but I can see the points in favor of using io errors without wrapping.

While SyntaxError is distinguishable, it is functionally less so than ErrUnexpectedEOF, which only requires a comparison instead of a type assertion.

Sounds to me like the Go2 error proposals would help a bit here :) Maybe we can wrap io errors in json.SyntaxError once/if those proposals are implemented.

It seems to me that changing to SyntaxError may actually be less invasive.

If it's just tests that break, and the breakage is consistent with what we're changing here, I'd just fix the tests. Unless what you mean is that unrelated tests are starting to fail. The cases you pasted do look like unexpected EOFs.

[agnivade]

Sure, I can fix the tests. Just needed a confirmation on whether to go ahead or not.

[montanaflynn]

I'm also hitting this, it's awkward to have to check if err == io.ErrUnexpectedEOF before a type assertion switch containing *json.SyntaxError.

[s3rj1k]

Any news on this?

[mvdan]

I think the solution here needs to be adapted given that error wrapping is a reality, e.g. see https://golang.org/pkg/errors/#Is.

SyntaxError should now wrap the underlying error, such as io.ErrUnexpectedEOF. And the json APIs should all be consistent with this.

If anyone wants to work on a patch and send it, I'm happy to review it. Otherwise, I don't think anyone is actively working on this, so there won't be any news unless someone leaves a comment with an update.

[mvdan]

I should also clarify - such a change is too risky now that we're in the middle of the 1.14 freeze, so a CL would probably be merged once the tree reopens in roughly six weeks from now.

[ali2210]

Sir I'm using go1.14

var member Member // struct type
err := json.NewDecoder(request.Body).Decode(&member); if err != nil{println(err)} // yeah this return eof & I expected value