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.

Sign up for GitHub

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

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

Merged
merged 13 commits into from
Jul 9, 2018
Merged

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

Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
48ef475
deprecate stream, move documentation to client|server stream
jeanbza Jul 3, 2018
d2c7992
Add ServerStream note about status package compatibility
jeanbza Jul 3, 2018
8d3bded
gofmt
jeanbza Jul 3, 2018
2d72a6a
add back the stream methods
jeanbza Jul 3, 2018
dc3de61
address doug's review
jeanbza Jul 4, 2018
385c62f
few more of doug's review comments that I missed
jeanbza Jul 4, 2018
2c1099b
DEPRECATED -> Deprecated
jeanbza Jul 4, 2018
25c227e
run goimports on stream.go
jeanbza Jul 4, 2018
0524d35
add back SendMsg
jeanbza Jul 6, 2018
50496f7
a few more doug review fixes
jeanbza Jul 9, 2018
19d4c65
add back io.EOF documentation
jeanbza Jul 9, 2018
3e34a01
adjust ServerStream.RecvMsg documentation
jeanbza Jul 9, 2018
5d8a458
remove promise about recvmsg in server
jeanbza Jul 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 68 additions & 40 deletions stream.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,43 +59,20 @@ type StreamDesc struct {

// Stream defines the common interface a client or server stream has to satisfy.
//
// All errors returned from Stream are compatible with the status package.
// Deprecated: See ClientStream and ServerStream documentation instead.
type Stream interface {
// Context returns the context for this stream. If called from the client,
// Should be done after Header or RecvMsg. Otherwise, retries may not be
// possible to perform.
// Deprecated: See ClientStream and ServerStream documentation instead.
Context() context.Context
// SendMsg is generally called by generated code. On error, SendMsg aborts
// the stream and returns an RPC status on the client side. On the server
// side, it simply returns the error to the caller.
//
// SendMsg blocks until:
// - It schedules m with the transport, or
// - The stream is done, or
// - The stream breaks.
//
// 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 closing the 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.
// Deprecated: See ClientStream and ServerStream documentation instead.
SendMsg(m interface{}) error
// RecvMsg blocks until it receives a message or the stream is
// done. On client side, it returns io.EOF when the stream is done. On
// any other error, it aborts the stream and returns an RPC status. On
// server side, it simply returns the error to the caller.
// It's 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.
// Deprecated: See ClientStream and ServerStream documentation instead.
RecvMsg(m interface{}) error
}

// ClientStream defines the interface a client stream has to satisfy.
// ClientStream defines the client-side behavior of a streaming RPC.
//
// All errors returned from ClientStream methods are compatible with the
// status package.
type ClientStream interface {
// Header returns the header metadata received from the server if there
// is any. It blocks if the metadata is not ready to read.
Expand All @@ -107,13 +84,35 @@ type ClientStream interface {
// CloseSend closes the send direction of the stream. It closes the stream
// when non-nil error is met.
CloseSend() error
// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Context() context.Context
// SendMsg is generally called by generated code. 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.
//
// SendMsg blocks until:
// - There is sufficient flow control to schedule m with the transport, or
// - The stream is done, or
// - 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,
// users should ensure the RPC completed successfully using RecvMsg.
//
// Always call Stream.RecvMsg() to drain the stream and get the final
// status, otherwise there could be leaked resources.
Stream
// 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 on the same stream in different goroutines.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"to call SendMsg or CloseSend"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Good call.

SendMsg(m interface{}) error
// RecvMsg blocks until it receives a message into m or the stream is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You deleted SendMsg by mistake.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who needs it! :)

Jk, added back in.

// done. It returns io.EOF when the stream completes successfully. On
// any other error, 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.
RecvMsg(m interface{}) error
}

// NewStream creates a new Stream for the client side. This is typically
Expand Down Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Ditto for ServerStream and Stream?

func NewClientStream(ctx context.Context, desc *StreamDesc, cc *ClientConn, method string, opts ...CallOption) (ClientStream, error) {
return cc.NewStream(ctx, desc, method, opts...)
}
Expand Down Expand Up @@ -840,7 +839,10 @@ func (a *csAttempt) finish(err error) {
a.mu.Unlock()
}

// ServerStream defines the interface a server stream has to satisfy.
// ServerStream defines the server-side behavior of a streaming RPC.
//
// All errors returned from ServerStream methods are compatible with the
// status package.
type ServerStream interface {
// SetHeader sets the header metadata. It may be called multiple times.
// When call multiple times, all the provided metadata will be merged.
Expand All @@ -856,7 +858,33 @@ type ServerStream interface {
// SetTrailer sets the trailer metadata which will be sent with the RPC status.
// When called more than once, all the provided metadata will be merged.
SetTrailer(metadata.MD)
Stream
// Context returns the context for this stream.
Context() context.Context
// SendMsg sends a message. On error, SendMsg aborts the stream and the
// error is returned directly.
//
// SendMsg blocks until:
// - There is sufficient flow control to schedule m with the transport, or
// - The stream is done, or
// - The stream breaks.
//
// SendMsg does not wait until the message is received by the server. An
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/server/client/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Good find.

// untimely stream closure may result in lost messages. To ensure delivery,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping on this comment..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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."

// users should ensure the RPC completed successfully using RecvMsg.
//
// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. (that make sense)

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nil, I assume?

(changed it as such on that assumption)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait...did I write that?

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - added back in io.EOF documentation.

Heh no prob :)

// any other error, 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.
RecvMsg(m interface{}) error
}

// serverStream implements a server side Stream.
Expand Down