Streaming forward handler fix chunk encoding

[emcfarlane]

Go http lib handles chunking of messages. This fixes encoding which corrupts non text messages.

[achew22]

Could you add a handler_test.go and exercise this in a way that demonstrates the bug to help prevent future regressions?

[emcfarlane]

@achew22 I added a test, let me know what needs to be updated. Also whats the reason for {"result": ...} in the JSON response?

[achew22]

That's a good question! I didn't write that code so I'm performing code archeology here, but I think the logic is that you don't want to return the proto directly so that you can put additional information in later without breaking people.

If we wanted to add "code", a status code for the request, you could do that safely now but you couldn't if that weren't the case.

Looks good to me! Thanks for your contribution

[aelsabbahy]

The JSON stream output is no longer newline separated, is this intentional?

[emcfarlane]

@aelsabbahy yes. Is this behaviour breaking something for you?

[aelsabbahy]

I happened to go get this project to try it out after this change was merged.

The readme needs to be updated if this is the way it'll work moving forward:
https://github.com/grpc-ecosystem/grpc-gateway#supported

Mapping streaming APIs to newline-delimited JSON

Also, what's the best way to parse the response now that newline is no longer a delimiter? I'm also noticing https://github.com/tmc/grpc-websocket-proxy might not be working well with this change messages only send at the end, rather than one at a time. But I'm only currently testing with https://github.com/danielstjules/wsc so maybe not a full end-end test.

[tmc]

Yes my proxy relies on newline separation.

[achew22]

I think the fix here is to additionally emit a "\n" into the stream to cause the data to have the newline delimit since returning to the old code would break the encoding and reopen this bug. Does anyone object to that?

I don't have an environment in which I can test that easily. @aelsabbahy, could you validate my theory by adding

if _, err = w.Write([]byte("\n")); err != nil {
	grpclog.Printf("Failed to add newline delimit for chunk: %v", err)
	return
}

in runtime/handler.go on line 64 (right after the existing w.Write')?

[emcfarlane]

This is a breaking behaviour. Can this be put behind a check for json content-type, or being part of the marshaller? This was breaking my code by not being able to control encoding.

[emcfarlane]

@achew22 shall I add it to this change #482 ?

[achew22]

Let's do it in a 2nd PR. Does my proposal fix it?

[emcfarlane]

@achew22 Its not very clean but would emcfarlane@5a75208 work? The issue is when not using JSON in the stream. I have a custom marshaler that works similarly to the new ProtoMarhshaler 0395325 but removes the stream map[]s added by https://github.com/grpc-ecosystem/grpc-gateway/blob/master/runtime/handler.go#L149 .

[aelsabbahy]

(off topic)
@afking any chance you can share your custom marshaler? I would be interested in that for a project I'm working on. Want to get rid of the result key in the map.

(On topic)
Can the marshal provider own/provide the delimiter value? This way application/json and \n aren't fixed values in the handler code.

[emcfarlane]

@aelsabbahy could check if the marshaler satisfies a Delimiter() interface?