deprecate stream, move documentation to client|server stream #2198

jeanbza · 2018-07-03T20:30:32Z

Deprecate Stream, and move the methods and documention to ServerStream
and ClientStream. This is due to the fact that there are different
semantics for SendMsg, and it's quite confusing to document one method
for two things. Furthermore, Stream is not actually used in any way
other than to be inherited by ClientStream and ServerStream.

Relevant issue: #2159

Deprecate Stream, and move the methods and documention to ServerStream and ClientStream. This is due to the fact that there are different semantics for SendMsg, and it's quite confusing to document one method for two things. Furthermore, Stream is not actually used in any way other than to be inherited by ClientStream and ServerStream. Relevant issue: grpc#2159

dfawley

Thanks for doing this, I think it will be a big help!

dfawley · 2018-07-03T21:20:28Z

stream.go

-	// But it is not safe to call RecvMsg on the same stream in different goroutines.
-	RecvMsg(m interface{}) error
-}
+// DEPRECATED: See ClientStream and ServerStream documentation instead.


Nit: "Deprecated" (case) - it actually matters to tools like staticcheck.

dfawley · 2018-07-03T21:21:16Z

stream.go


 // ClientStream defines the interface a client stream has to satisfy.
+// All errors returned from ClientStream are compatible with the status package.


Nit: "from ClientStream methods"

(and ServerStream)

Done, and applied similar change to ServerStream.

dfawley · 2018-07-03T21:22:55Z

stream.go

-	RecvMsg(m interface{}) error
-}
+// DEPRECATED: See ClientStream and ServerStream documentation instead.
+type Stream interface {}

 // ClientStream defines the interface a client stream has to satisfy.


"ClientStream defines the client-side behavior of a streaming RPC."?

Done, and applied a similar change to ServerStream.

dfawley · 2018-07-03T21:26:02Z

stream.go

+	// Context returns the context for this stream.
+	Context() context.Context
+	// SendMsg is generally called by generated code. On error, SendMsg aborts
+	// the stream and returns an io.EOF; the status may be determined by


"On error, SendMsg aborts the stream. If the error was generated by the client, the status is returned directly; otherwise, io.EOF is returned and the status of the stream may be discovered using RecvMsg."

dfawley · 2018-07-03T21:26:55Z

stream.go

+	// inspecting the error returned by calling RecvMsg.
+	//
+	// SendMsg blocks until:
+	//   - It schedules m with the transport, or


How about: "- There is sufficient flow control to schedule m with the transport, or"

...or something to indicate that we aren't just letting people queue up a billion messages.

dfawley · 2018-07-03T21:33:51Z

stream.go

+	// SendMsg does not wait for an ack. An untimely stream closure can result
+	// in any buffered messages along the way (including those in the
+	// client-side buffer that comes with gRPC by default) being lost. To
+	// ensure delivery, users must call RecvMsg until receiving an EOF before


This is painfully explicit IMO. How about:

"SendMsg does not wait until the message is received by the server. An untimely stream closure may result in lost messages. To ensure delivery, users should ensure the RPC completed successfully using RecvMsg."

The idea would be to refer people to RecvMsg and make sure we document it well enough there.

That is really good. 👍

Done.

dfawley · 2018-07-03T21:34:20Z

stream.go

-	Stream
+	// It's safe to have a goroutine calling SendMsg and another goroutine
+	// calling RecvMsg on the same stream at the same time. It is not safe
+	// to call SendMsg on the same stream in different goroutines.


"to call SendMsg or CloseSend"

Done. Good call.

dfawley · 2018-07-03T21:36:50Z

stream.go

+	// calling RecvMsg on the same stream at the same time. It is not safe
+	// to call SendMsg on the same stream in different goroutines.
+	SendMsg(m interface{}) error
+	// RecvMsg blocks until it receives a message or the stream is done. On


Let's remove the client/server language:

RecvMsg blocks until it receives a message into m or the stream is done. It returns io.EOF when the stream completes successfully. On any other error, it the stream is aborted and the error contains the RPC status.

It is safe to have a goroutine calling SendMsg and another goroutine calling RecvMsg on the same stream at the same time, but it is not safe to call RecvMsg on the same stream in different goroutines.

dfawley · 2018-07-03T21:38:17Z

stream.go

+	//   - The stream is done, or
+	//   - The stream breaks.
+	//
+	// SendMsg does not wait for an ack. An untimely stream closure can result


Same comment as above.

dfawley · 2018-07-03T21:39:02Z

stream.go

+	// calling RecvMsg on the same stream at the same time. It is not safe
+	// to call SendMsg on the same stream in different goroutines.
+	SendMsg(m interface{}) error
+	// RecvMsg blocks until it receives a message or the stream is done. On


Please remove client/server language here.

Done (copy paste from above).

jeanbza · 2018-07-04T00:12:43Z

All comments addressed; PTAL at your convenience.

jeanbza · 2018-07-04T00:14:08Z

Wait, ignore that, github was hiding ones from me. Fixing the outstanding ones!

dfawley · 2018-07-06T17:13:44Z

stream.go

+	// It's safe to have a goroutine calling SendMsg and another goroutine
+	// calling RecvMsg on the same stream at the same time. It is not safe
+	// to call SendMsg on the same stream in different goroutines.
+	// RecvMsg blocks until it receives a message into m or the stream is


You deleted SendMsg by mistake.

Who needs it! :)

Jk, added back in.

dfawley · 2018-07-06T19:41:45Z

stream.go

@@ -145,7 +144,7 @@ func (cc *ClientConn) NewStream(ctx context.Context, desc *StreamDesc, method st

 // NewClientStream is a wrapper for ClientConn.NewStream.
 //
-// DEPRECATED: Use ClientConn.NewStream instead.
+// Deprecated: Use ClientConn.NewStream instead.


Let's not change this just yet...generated code still uses this (so vet fails).

Done. Ditto for ServerStream and Stream?

dfawley · 2018-07-06T19:47:10Z

stream.go

-	// Stream.SendMsg() may return a non-nil error when something wrong happens sending
-	// the request. The returned error indicates the status of this sending, not the final
-	// status of the RPC.
+	// Context returns the context for this stream.


Sorry...can you add this, too (only for ClientStream):

It should not be called until after Header or RecvMsg has returned. Once called, subsequent client-side retries are disabled.

Done. Good note - that's unexpected (to me).

dfawley · 2018-07-06T19:48:53Z

stream.go

+	//   - The stream is done, or
+	//   - The stream breaks.
+	//
+	// SendMsg does not wait until the message is received by the server. An


s/server/client/

Done. Good find.

dfawley · 2018-07-06T19:50:47Z

stream.go

+	//   - The stream breaks.
+	//
+	// SendMsg does not wait until the message is received by the server. An
+	// untimely stream closure may result in lost messages. To ensure delivery,


The server can never know whether the client received the message without something in the user's protocol.

Right - this line alludes to that. I could more strongly state that by adjusting the line to:

"SendMsg does not wait until the message is received by the client. Users who need per-message acknowledgement are expected to implement it on top of gRPC."

Though, I don't know how much value there is in that. I kind of think the brevity of just saying, "we don't wait" is enough of a warning. Thoughts?

Will you update this? The bit about "ensure the RPC completed successfully using RecvMsg" is misleading/wrong on the server-side.

Ping on this comment..

Shoot, sorry to have missed this. Hm, good point - that just means the client has finished sending their messages. Bad copy paste. I'm not sure it's worth saying, but is it correct to say that once the server returns in the handler, all messages will have been sent successfully? Or is the promise still only that messages have been scheduled? In short, is there any way to ensure all server messages have arrived? (I think not, but want to make sure I ask)

In the meantime, I've simply removed the line about RecvMsg. I neglected to include the line about per-message acks, since it doesn't seem to add too much more.

I'm not sure it's worth saying, but is it correct to say that once the server returns in the handler, all messages will have been sent successfully?

No, the server never knows whether a message is received by the client unless the user builds their own acks into their protocol. I think what you were proposing before was fine: "SendMsg does not wait until the message is received by the client."

dfawley · 2018-07-06T19:51:19Z

stream.go

+	// to call SendMsg or CloseSend on the same stream in different goroutines.
+	SendMsg(m interface{}) error
+	// RecvMsg blocks until it receives a message into m or the stream is
+	// done. It returns io.EOF when the stream completes successfully. On


On the server, io.EOF should not be returned.

nil, I assume?

(changed it as such on that assumption)

wait...did I write that?

RecvMsg does return io.EOF, but it's when the client does CloseSend, not when the stream completes. Sorry.

Done - added back in io.EOF documentation.

Heh no prob :)

dfawley · 2018-07-06T19:52:12Z

stream.go

+	//
+	// It is safe to have a goroutine calling SendMsg and another goroutine
+	// calling RecvMsg on the same stream at the same time, but it is not safe
+	// to call SendMsg or CloseSend on the same stream in different goroutines.


The "or CloseSend" only applies to the client, not the server. The server doesn't have a CloseSend. Sorry if I put my comment in the wrong place earlier.

Done. (that make sense)

dfawley · 2018-07-09T17:31:12Z

stream.go

+	// to call SendMsg on the same stream in different goroutines.
+	SendMsg(m interface{}) error
+	// RecvMsg blocks until it receives a message into m or the stream is done.
+	// It returns io.EOF when the stream completes successfully. On any non-EOF


"It returns io.EOF when the client has performed a CloseSend."?

jeanbza

.

jeanbza added 2 commits July 3, 2018 13:29

Add ServerStream note about status package compatibility

d2c7992

jeanbza requested a review from dfawley July 3, 2018 20:30

jeanbza added 2 commits July 3, 2018 14:19

gofmt

8d3bded

add back the stream methods

2d72a6a

dfawley reviewed Jul 3, 2018

View reviewed changes

dfawley added the Type: Documentation Documentation or examples label Jul 3, 2018

dfawley added this to the 1.14 Release milestone Jul 3, 2018

address doug's review

dc3de61

jeanbza added 3 commits July 3, 2018 17:16

few more of doug's review comments that I missed

385c62f

DEPRECATED -> Deprecated

2c1099b

run goimports on stream.go

25c227e

dfawley reviewed Jul 6, 2018

View reviewed changes

add back SendMsg

0524d35

dfawley reviewed Jul 6, 2018

View reviewed changes

jeanbza added 2 commits July 9, 2018 08:44

a few more doug review fixes

50496f7

add back io.EOF documentation

19d4c65

dfawley reviewed Jul 9, 2018

View reviewed changes

jeanbza added 2 commits July 9, 2018 13:05

adjust ServerStream.RecvMsg documentation

3e34a01

remove promise about recvmsg in server

5d8a458

jeanbza commented Jul 9, 2018

View reviewed changes

dfawley approved these changes Jul 9, 2018

View reviewed changes

jeanbza merged commit 50de898 into grpc:master Jul 9, 2018

lock bot locked as resolved and limited conversation to collaborators Jan 6, 2019


		// ClientStream defines the interface a client stream has to satisfy.
		// All errors returned from ClientStream are compatible with the status package.

deprecate stream, move documentation to client|server stream #2198

deprecate stream, move documentation to client|server stream #2198

Conversation

jeanbza commented Jul 3, 2018 • edited Loading

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanbza commented Jul 4, 2018

jeanbza commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanbza left a comment • edited Loading

Choose a reason for hiding this comment

jeanbza commented Jul 3, 2018 •

edited

Loading

jeanbza left a comment •

edited

Loading