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

encoding/json: document and limit buffering #1955

Closed
gopherbot opened this issue Jun 14, 2011 · 12 comments

Comments

Projects
None yet
5 participants
@gopherbot
Copy link

commented Jun 14, 2011

by jdnurmi@qwe.cc:

What steps will reproduce the problem?
1. Compile & run the attached

What is the expected output?
"Everything is A-OK"

What do you see instead?
"Didn't get proper exdata, got ''"

Which compiler are you using (5g, 6g, 8g, gccgo)?
8g

Which operating system are you using?
linux

Which revision are you using?  (hg identify)
tip

Please provide any additional information below.

I believe the cause is that the decoder creates (and grows) a buffer without confirming
that the total amount is needed.  This approach is probably far more efficient than a
correct 'byte.Read && process || byte.Unread' approach, but it would be nice if
there were an exported 
json.DecodeElement(io.Reader, interface{})(os.Error) that didn't "over-feed".

Attachments:

  1. json_bug.go (465 bytes)
@peterGo

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2011

Comment 1:

func (*Decoder) Decode
Decode reads the next JSON-encoded value from its input and stores it in the value
pointed to by v.
http://golang.org/pkg/json/#Decoder.Decode
func (*Buffer) Read
Read reads the next len(p) bytes from the buffer or until the buffer is drained.
http://golang.org/pkg/bytes/#Buffer.Read
When json.Decode reads from bytes.Buffer (buf.Read), the bytes in the buffer are
consumed.
package main
import (
    "bytes"
    "json"
    "log"
)
func main() {
    buf := bytes.NewBufferString(`{"k":"v"}exdata`)
    m := map[string]string{}
    log.Println("buf.Len:", buf.Len())
    err := json.NewDecoder(buf).Decode(&m)
    if err != nil {
        log.Fatalf("Couldn't even decode...")
    }
    log.Println("buf.Len:", buf.Len())
    exbuff := make([]byte, 80)
    n, err := buf.Read(exbuff)
    if string(exbuff[0:n]) != "exdata" {
        log.Fatalf("Didn't get proper exdata, got '%s'", exbuff[0:n])
    }
    log.Printf("Everything was A-OK")
}
Output:
2011/06/13 23:45:02 buf.Len: 15
2011/06/13 23:45:02 buf.Len: 0
2011/06/13 23:45:02 Didn't get proper exdata, got ''
@gopherbot

This comment has been minimized.

Copy link
Author

commented Jun 14, 2011

Comment 2 by jdnurmi@qwe.cc:

I totally agree that is what's happening;  The bug (as I view it) is that the json
element should be decoded only to the end of the object ('}');  At that point, I should
be able to resume the read and read 'exdata' (non-JSON data) if so desired.
It's not the standard JSON stream -- but it doesn't seem unreasonable to read a JSON
element and not 'extra' non-JSON data.
Or did I misunderstand your comment?
@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2011

Comment 3:

We should fix this.
Note that in this specific case JSON can do without reading ahead
but in general it cannot.  If the JSON value is 123, it has to
consume the byte that follows in order to find out that the
number is over.

Status changed to Accepted.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 16, 2011

Comment 4:

http.ReadRequest takes a *bufio.Reader.  JSON could also?  Then you could Peek at it.
Perhaps add json.NewBufferedDecoder() and keep json.NewDecoder() as a wrapper.
Or a fun technique I used recently:  you can have json.Decoder.Decode return not just an
os.Error but instead a (remaining io.Reader, err os.Error) where remaining is:
   remaining = io.MultiReader(bytes.NewBuffer(slop), originalReader)
@rsc

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2011

Comment 5:

I think the fix is to do what flate and gob do: define that
if the input has a ReadByte then more buffering won't be added.

Status changed to HelpWanted.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2011

Comment 7:

Labels changed: added priority-later.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2011

Comment 8:

Labels changed: added priority-go1.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2012

Comment 9:

Owner changed to builder@golang.org.

@gopherbot

This comment has been minimized.

Copy link
Author

commented Feb 3, 2012

Comment 11 by monnand:

I submitted a CL to solve this issue. Ready to be reviewed:
http://golang.org/cl/5623053/
@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2012

Comment 12:

This issue was closed by revision 49110ea.

Status changed to Fixed.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 23, 2013

Comment 13:

Proposed addition for this: https://golang.org/cl/7181053/
@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 29, 2013

Comment 14:

This issue was updated by revision 91e99c1.

R=golang-dev, adg, rsc
CC=golang-dev
https://golang.org/cl/7181053

@rsc rsc added this to the Go1 milestone Apr 10, 2015

@rsc rsc removed the priority-go1 label Apr 10, 2015

@golang golang locked and limited conversation to collaborators Jun 24, 2016

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.