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.

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

proposal: encoding/json: accept and use io.ByteReader or expose input buffering #8658

Closed
gopherbot opened this issue Sep 4, 2014 · 8 comments
Labels
FrozenDueToAge v2 A language change or incompatible library change
Milestone

Comments

@gopherbot
Copy link

http://golang.org/pkg/encoding/json/#Decoder.Buffered is quite painful to use, if your
protocol e.g. alternates between other things and json. It would be nice if one could
pass a bufio.Reader to NewDecoder, and be able to share the bufio buffering between
calls to Decode, and other protocol logic.

I don't expect this to change quickly. But perhaps it could use bufio right, without
breaking the Decoder.Buffered guarantee for Go1?

At least, once you do break Go1 promises, please change this.
@ianlancetaylor
Copy link
Contributor

Comment 1:

Labels changed: added repo-main, go2.

@gopherbot gopherbot added new v2 A language change or incompatible library change labels Sep 4, 2014
@bradfitz bradfitz removed the new label Dec 18, 2014
@rsc rsc changed the title encoding/json: Should use bufio instead of it's own buffering logic & Decoder.Buffered encoding/json: use bufio Apr 10, 2015
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc removed the repo-main label Apr 14, 2015
@rsc rsc changed the title encoding/json: use bufio encoding/json: accept and use io.ByteReader or expose input buffering Jun 17, 2017
@rsc rsc changed the title encoding/json: accept and use io.ByteReader or expose input buffering proposal: encoding/json: accept and use io.ByteReader or expose input buffering Jun 17, 2017
@ianlancetaylor
Copy link
Contributor

I think it is possible today to pass r, an io.Reader, to json.NewDecoder to get d, parse some data using d, then do r = io.MultiReader(d.Buffered(), r) to read the subsequent non-JSON data.

Perhaps it would make sense to develop a set of conventions for handling a single byte stream that contains different kinds of data, with buffer sharing. The problem in general seems larger than JSON. But this issue doesn't describe a general approach, and I don't have any ideas myself.

Since this specific problem can be handled reasonably today, and since the larger problem needs a proposal, closing this issue.

@tv42
Copy link

tv42 commented Jan 4, 2018

I don't see what bigger proposal is needed than "accept *bufio.Reader, only consume the data you really need to consume". That way, you can pass the same *bufio.Reader in turn to decoders of various formats.

As far as I can see, this part of the encoding/json API is more like a historical accident, and it could use *bufio.Reader underneath, only returning >0 bytes from .Buffered() when the caller passed in a Reader that wasn't already a *bufio.Reader.

@ianlancetaylor
Copy link
Contributor

It's not clear that we want to require buffering where it is not appropriate. Accepting a simple io.Reader interface is a standard Go approach; changing all streaming packages to use a fixed type rather than an interface does not sound right to me. However, I encourage you to write that up as a separate proposal if you think it is the best approach.

@tv42
Copy link

tv42 commented Jan 4, 2018

I'm not suggesting to change json.NewDecoder argument types. I'm suggesting it check if it was passed a *bufio.Reader, and if so use that for buffering. (If not, it can call bufio.NewReader itself, and store a bool that it did so; that matters for .Buffered behavior.)

In fact, other decoders in stdlib do just that, and don't even offer the Buffered api. The check-and-reuse is delegated to bufio.NewReader{,Size} https://golang.org/src/bufio/bufio.go?s=1304:1354#L36 and not really ever documented :-( For example, csv.NewReader: https://golang.org/src/encoding/csv/reader.go?s=4576:4611#L125

@ianlancetaylor
Copy link
Contributor

I encourage you to write that up as a separate proposal.

@tv42
Copy link

tv42 commented Jan 4, 2018

sigh ... this was that proposal ...

I mean, I could use more words, for argumentation, but the "separate" part comes off very weird.

@ianlancetaylor
Copy link
Contributor

This proposal was written as specific to JSON. It says that Buffered is hard to use, which I do not agree with.

I'm saying that if you want to change something, it should be broader than JSON, and it should not have anything to do with a Buffered method that would presumably no longer be needed. It should be written as a general approach that all streaming interfaces should adopt. Right now streaming interfaces are simple: they use io.Reader. You are suggesting something different, something that I assume applies to streaming interfaces that need to look ahead. We would need clear rules for how to invoke those interfaces and clear rules for how they should preserve data they have not yet read. There might need to be some sort of synchronization to make clear exactly when the input source is available to read different kinds of data.

I don't see any of that in this proposal. All this proposal says is to use bufio.Reader. That is not enough.

@golang golang locked and limited conversation to collaborators Jan 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

5 participants