Add bidirectional streaming support by running Send() and Recv() concurrently #170

tmc · 2016-05-27T00:40:17Z

Fixes #169

yugui · 2016-05-30T01:34:40Z

protoc-gen-grpc-gateway/gengateway/template.go

+		return nil, metadata, err
+	}
+	metadata.HeaderMD = header
+	return stream, metadata, <-sendErrs


it blocks forever unless you close sendErrs?

it still blocks until the end of the goroutine even if you close sendErrs. So it is not interleaving?

my intent was to block until the message is sent by the client as a sort of middle ground between just ignoring errors and interrupting the stream on any error. This was written mid-migraine so I'm happy to admit it's not as thought out as it could be. Open to suggestions.

I overlooked L260-L263. So actually it does not block forever.

I have two more questions.

Maybe I'm missing something, but it does not seem to be something like "interleave" for me. What do you mean by "interleaving Send and Recv"?

If it is intended to block until the message is sent, why did you need to run the function in a goroutine?
IIUC, the only difference made by the goroutine is that the goroutine function runs in parallel to stream.Header().
But I don't think the difference makes much sense.

This is for the many messages situation (e.g. rpc Chat(stream ChatMessage) returns (stream ChatMessage)). Before this change all Send()s have to run before Recv() gets called. by putting Send() in a goroutine concurrent Send() and Recv() calls can be made.

tmc · 2016-06-02T02:57:56Z

@yugui do you need further clarification?

To restate, before this change a streaming handler did not start a goroutine and ran all Send()s before returning and having the caller call Recv(). This proposal is a middle ground of blocking until the first Send() to aid in surfacing issues with initial payloads.

Perhaps setting errs to nil would be a nicer way to express the semantics of block until first execution. I'm open to suggestion here.

yugui · 2016-06-02T05:35:36Z

a streaming handler did not start a goroutine and ran all Send()s before returning and having the caller call Recv().

I don't think it was changed with this PR because <-sendErrs waits for an error from the goroutine or the termination of the goroutine. Also on error from <-sendErrs, the caller returns wihtout calling Recv. So Send and Recv never interleave, IIUC.

So in my understanding, the change made by this PR was

Read req.Body until its end even if there was an error
Send streams to the upstream grpc server even if there was an error

I think your intention

to block until the message is sent by the client as a sort of middle ground between just ignoring errors and interrupting the stream on any error.

makes sense. But if my understanding above was correct, you didn't need the new goroutine. You could do the same thing in the same goroutine as caller.

tmc · 2016-06-02T06:07:11Z

How would you accomplish this without a separate goroutine?

On Jun 1, 2016, at 10:35 PM, Yuki Yugui Sonoda notifications@github.com wrote:

a streaming handler did not start a goroutine and ran all Send()s before returning and having the caller call Recv().

I don't think it was changed with this PR because <-sendErrs waits for an error from the goroutine or the termination of the goroutine. Also on error from <-sendErrs, the caller returns wihtout calling Recv. So Send and Recv never interleave, IIUC.

So in my understanding, the change made by this PR was

Read req.Body until its end even if there was an error
Send streams to the upstream grpc server even if there was an error
I think your intention

to block until the message is sent by the client as a sort of middle ground between just ignoring errors and interrupting the stream on any error.

makes sense. But if my understanding above was correct, you didn't need the new goroutine. You could do the same thing in the same goroutine as caller.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

tmc · 2016-06-02T06:09:56Z

Did you miss that the first Send if successful will send nil and stop blocking?

On Jun 1, 2016, at 10:35 PM, Yuki Yugui Sonoda notifications@github.com wrote:

a streaming handler did not start a goroutine and ran all Send()s before returning and having the caller call Recv().

I don't think it was changed with this PR because <-sendErrs waits for an error from the goroutine or the termination of the goroutine. Also on error from <-sendErrs, the caller returns wihtout calling Recv. So Send and Recv never interleave, IIUC.

So in my understanding, the change made by this PR was

Read req.Body until its end even if there was an error
Send streams to the upstream grpc server even if there was an error
I think your intention

to block until the message is sent by the client as a sort of middle ground between just ignoring errors and interrupting the stream on any error.

makes sense. But if my understanding above was correct, you didn't need the new goroutine. You could do the same thing in the same goroutine as caller.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

yugui · 2016-06-02T06:40:45Z

Did you miss that the first Send if successful will send nil and stop blocking?

Yes, I did. Sorry.

Then, why do you treat the first item in the stream specially?
It returns 400 Bad Request if it fails to decode the first item but it keeps going when it fails to decode later items.

Also I'm not sure if it is safe to start writing response body before reading the request body until the end.

https://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html#sec6

After receiving and interpreting a request message, a server responds with an HTTP response message.

… calls.

tmc · 2016-06-07T19:47:30Z

@yugui I was missing some things here (it was written in a rush). This now ensures CloseSend is called and cleans up the semantics a bit.

One alternative would be to move the Send handling out and invoke it once to capture the initial error. Thoughts?

tmc · 2016-06-07T20:16:47Z

@yugui I changed the implementation to be more clear.

yugui · 2016-06-13T06:13:27Z

@tmc Thank you. That's much clearer.
I still have small coding-level comments and I am worrying about compatibility to the current HTTP/1.1 version of response forwarder.

https://www.w3.org/Protocols/rfc2616/rfc2616-sec6.html#sec6

After receiving and interpreting a request message, a server responds with an HTTP response message.

In my understanding, it requires us to receive the request before sending any response in the TCP session. So #168 looks to be a requirement of this PR. What do you think?

yugui · 2016-06-13T06:14:44Z

protoc-gen-grpc-gateway/gengateway/template.go

+	dec := marshaler.NewDecoder(req.Body)
+	handleSend := func() error {
+		var protoReq {{.Method.RequestType.GoType .Method.Service.File.GoPkg.Path}}
+		err = dec.Decode(&protoReq)


if err := dec.Decode(&protoReq); err != nil would be more consistent?

i was following existing style from the 'client-streaming-request-func' template.

tmc · 2016-06-13T06:57:26Z

@yugui updated with feedback taken into consideration.

This SO post deals with the state of response-before-request (early 401s in particular) http://stackoverflow.com/questions/14250991/is-it-acceptable-for-a-server-to-send-a-http-response-before-the-entire-request

The most common case here would be clients that don't process the response until they've sent the request fully. For my use case (a websocket proxy on top of grpc-gateway) this isn't a concern as I control the http communication on both sides.

I don't think having some misbehaving http clients should block this as it enables writing websocket proxies. Having this work in grpc-gateway closes the gap of bringing grpc semantics to browsers (with a little extra effort).

yugui · 2016-06-13T07:08:22Z

@tmc. Thank you. LGTM.

karlkfi · 2016-06-18T20:40:00Z

New to grpc-gateway. Have some questions related to this work.

I take it by "enables writing websocket proxies" that grpc-gateway does not itself use websockets for streaming. Is that correct?

Is there an external websocket proxy project?

If not, what would one look like? Would it be generic or have to be generated from proto files?

tmc · 2016-06-18T21:24:46Z

@karlkfi that is correct.

I have an example websocket proxy here: https://github.com/tmc/grpcutils/blob/master/websocket_proxy.go

it's a generic http.Handler that will perform a ws upgrade on the same url as the http endpoint.

example use here:
https://github.com/tmc/grpcutils/tree/master/examples/cmd/wsechoserver

Add bidirectional streaming support by running Send() and Recv() concurrently

tmc force-pushed the concurrent_forward branch from cb23d41 to f4d972a Compare May 27, 2016 00:52

yugui reviewed May 30, 2016
View reviewed changes

tmc changed the title ~~Add bidirectional streaming support by interleaving Send() and Recv()~~ Add bidirectional streaming support by running Send() and Recv() concurrently Jun 2, 2016

tmc added 2 commits June 7, 2016 12:45

Add bidirectional streaming support by interleaving Send() and Recv()…

a5064d5

… calls.

ensure CloseSend is called, exit on errors

1725708

tmc force-pushed the concurrent_forward branch from 2a02af8 to 1725708 Compare June 7, 2016 19:45

eliminate confusing errs channel

d6f3500

tmc force-pushed the concurrent_forward branch from 6b70c26 to d6f3500 Compare June 7, 2016 20:45

yugui reviewed Jun 13, 2016
View reviewed changes

tmc added 2 commits June 12, 2016 23:45

return earlier when first Send fails

c6357e8

run realclean and examples targets with latest proto

6e38833

yugui merged commit 90a540e into grpc-ecosystem:master Jun 13, 2016

tmc deleted the concurrent_forward branch June 13, 2016 07:45

yudai mentioned this pull request Jul 12, 2017

Timing issue with /v3alpha/lease/keepalive that is trivially reproducible with telnet etcd-io/etcd#8237

Closed

adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020

Merge pull request grpc-ecosystem#170 from tmc/concurrent_forward

687fc9d

Add bidirectional streaming support by running Send() and Recv() concurrently

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bidirectional streaming support by running Send() and Recv() concurrently #170

Add bidirectional streaming support by running Send() and Recv() concurrently #170

tmc commented May 27, 2016

yugui May 30, 2016

tmc May 30, 2016

yugui May 30, 2016 •

edited

Loading

tmc May 30, 2016 •

edited

Loading

tmc commented Jun 2, 2016

yugui commented Jun 2, 2016

tmc commented Jun 2, 2016

tmc commented Jun 2, 2016

yugui commented Jun 2, 2016

tmc commented Jun 7, 2016

tmc commented Jun 7, 2016

yugui commented Jun 13, 2016

yugui Jun 13, 2016

tmc Jun 13, 2016

tmc commented Jun 13, 2016

yugui commented Jun 13, 2016

karlkfi commented Jun 18, 2016

tmc commented Jun 18, 2016

Add bidirectional streaming support by running Send() and Recv() concurrently #170

Add bidirectional streaming support by running Send() and Recv() concurrently #170

Conversation

tmc commented May 27, 2016

yugui May 30, 2016

Choose a reason for hiding this comment

tmc May 30, 2016

Choose a reason for hiding this comment

yugui May 30, 2016 • edited Loading

Choose a reason for hiding this comment

tmc May 30, 2016 • edited Loading

Choose a reason for hiding this comment

tmc commented Jun 2, 2016

yugui commented Jun 2, 2016

tmc commented Jun 2, 2016

tmc commented Jun 2, 2016

yugui commented Jun 2, 2016

tmc commented Jun 7, 2016

tmc commented Jun 7, 2016

yugui commented Jun 13, 2016

yugui Jun 13, 2016

Choose a reason for hiding this comment

tmc Jun 13, 2016

Choose a reason for hiding this comment

tmc commented Jun 13, 2016

yugui commented Jun 13, 2016

karlkfi commented Jun 18, 2016

tmc commented Jun 18, 2016

yugui May 30, 2016 •

edited

Loading

tmc May 30, 2016 •

edited

Loading