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: Encode channel as array #11940

Closed
lukescott opened this issue Jul 30, 2015 · 11 comments

Comments

Projects
None yet
6 participants
@lukescott
Copy link

commented Jul 30, 2015

I'm currency doing something like this to encode a channel:

if _, err = w.Write([]byte{'['}); err != nil {
    return err
}
enc := json.NewEncoder(w)
if err = enc.Encode(<-objects); err != nil {
    return err
}
for o := range objects {
    if _, err = w.Write([]byte{','}); err != nil {
        return err
    }
    if err = enc.Encode(o); err != nil {
        return err
    }
}
if _, err = w.Write([]byte{']'}); err != nil {
    return err
}

I have a couple issues with this approach:

1 - It's verbose
2 - enc.Encode terminates each object with a newline character, so I end up with something like:

[{}
,{}
,{}]

Instead of:

[{},{},{}]

Which is a bit odd because json.Marshal does not terminate with a newline character. I would have expected them both to do the same thing, just Marshal allocates a writer/buffer for you.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jul 30, 2015

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2015

I guess you are suggesting that enc.Encode not always terminate with a newline? Other than that there doesn't seem to be an issue here.

@lukescott

This comment has been minimized.

Copy link
Author

commented Jul 30, 2015

It would be nice if it didn't terminate with a newline, but I'm assuming that can't be changed, right?

What I'm suggesting in this issue is to support encoding a channel - encode it as an array as soon as values are received.

With my use-case I'm streaming a very large json document to a mobile client - which stream decodes the document. The goal is to never have the entire document in memory on the server or client. The server pushes each row onto the channel, which gets encoded on the wire, which gets decoded and inserted into a local database.

@lukescott

This comment has been minimized.

Copy link
Author

commented Jul 30, 2015

I wrote a utility method to handle this:

func jsonEncodeChan(w io.Writer, vc interface{}) (err error) {
    cval := reflect.ValueOf(vc)
    _, err = w.Write([]byte{'['})
    if err != nil {
        return
    }
    var buf *bytes.Buffer
    var enc *json.Encoder
    v, ok := cval.Recv()
    if !ok {
        goto End
    }
    // create buffer & encoder only if we have a value
    buf = new(bytes.Buffer)
    enc = json.NewEncoder(buf)
    goto Encode
Loop:
    v, ok = cval.Recv()
    if !ok {
        goto End
    }
    if _, err = w.Write([]byte{','}); err != nil {
        return
    }
Encode:
    err = enc.Encode(v.Interface())
    if err == nil {
        _, err = w.Write(bytes.TrimRight(buf.Bytes(), "\n"))
        buf.Reset()
    }
    if err != nil {
        return
    }
    goto Loop
End:
    _, err = w.Write([]byte{']'})
    return
}

That's how I would envision encoding/json handling a channel. Although it would be a bit simpler using the internal encode method (no \n or separate buffer needed).

@jpfielding

This comment has been minimized.

Copy link

commented Oct 19, 2016

was running into this today when trying to stream out json. if we had something like the following, the mapping into and and out of chans might be compatible with the corresponding slice of the same type. im going to look at the source and see if there is any hope of this working.

type SearchResult struct {
    Columns []string 'json:"columns"`
    Rows  chan []string `json:"work,size=10`
}

type SearchResult struct {
    Columns []string 'json:"columns"`
    Rows  [][]string `json:"work`
}
@gopherbot

This comment has been minimized.

Copy link

commented Nov 26, 2016

CL https://golang.org/cl/33591 mentions this issue.

@bradfitz bradfitz changed the title encoding/json: Encode channel as array proposal: encoding/json: Encode channel as array Nov 30, 2016

@bradfitz bradfitz modified the milestones: Proposal, Unplanned Nov 30, 2016

@bradfitz bradfitz added the Proposal label Nov 30, 2016

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2016

In general there are lots of ways a channel might expect to be used. This would hard-code one and break all the other ways.

Also people tend to think of Marshal as a read-only operation on the underlying data. Consuming from a channel would not be.

These two seem like serious problems, and I don't see a corresponding serious need. You can do this today by defining a custom channel type with a MarshalJSON method.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2016

Since no one argued against this, declining proposal.

@rsc rsc added Proposal-Declined and removed Proposal labels Dec 12, 2016

@rsc rsc closed this Dec 12, 2016

@lukescott

This comment has been minimized.

Copy link
Author

commented Dec 12, 2016

Sorry, I've had a busy week and haven't had a chance to respond to this.

@rsc could you expand on this:

In general there are lots of ways a channel might expect to be used. This would hard-code one and break all the other ways.

Currently a channel is not handled at all, correct? And if the default behavior would be to treat it as an array, wouldn't Marshaler/Unmarshaler override the default behavior?

The Marshaler interface also doesn't work for streaming because you have to hold the entire contents in memory before returning it:

// Note: This code is abbreviated and not tested
func (c ChannelType) MarshalJSON() ([]byte, error) {
    buf := new(bytes.Buffer)
    buf.WriteString("[")
    enc = json.NewEncoder(buf)
    for v := range c {
            enc.Encode(v)
	    buf.WriteString(",")
    }
    buf.WriteString("]")
    return buf.Bytes()
}

This makes streaming not possible with json's Marshaler. That's why I wrote the utility method in a previous post.

The encoding/xml Marshaler is actually more flexible:

type Marshaler interface {
        MarshalXML(e *Encoder, start StartElement) error
}

It passes the encoder so you can 1) add to the output stream, and 2) you don't have to define another encoder.

I would like to see a channel treated as an array... But if the Marshaler looked like encoding/xml's, it wouldn't be needed:

type Marshaler interface {
        MarshalJSON(e *Encoder) error
}

(The Encoder would have a method for writing tokens, or you could encode individual values)

Of course there is a compatibility issue with changing the interface.. which is why it was suggested to treat channel as an array. Which, if I'm not mistaken, is an error unless you write a Marshaler for it.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2016

Re uses: What if the channel is in the struct because it is for sending to, not receiving from? What if there are a known number of values to receive over the channel, and it will never be closed? Lots of things that could reasonably go wrong. Better to reject channels by default: an error message is better than a deadlock because the channel is being used wrong.

As for json streaming, the current encoder does not write any part of the JSON response to the underlying writer until the entire JSON is constructed. (Otherwise it might encounter some problem midway and return an error but have written a half-JSON to the underlying writer. It avoids this by buffering until it knows the JSON conversion has succeeded.)

Some people do want streaming, of course, and if we add streaming we should also add some way to stream during a custom encoding method. But that makes streaming a non-argument for built-in channel handling.

@lukescott

This comment has been minimized.

Copy link
Author

commented Dec 12, 2016

Ah, good points on the channels. I did not think about that.

If we had a way to do streaming without involving channels, that would solve the problem for me 👍 .

Is there a proposal that covers the Marshaler? Does one need to be created?

@rsc

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2016

I don't believe there is any proposal, but I think we'd rather see that developed as an external package first.

@golang golang locked and limited conversation to collaborators Dec 13, 2017

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.