grpc-go icon indicating copy to clipboard operation
grpc-go copied to clipboard

New CallOption: grpc.ManualCancel

Open dfawley opened this issue 7 years ago • 4 comments

We fork a goroutine for every stream here:

https://github.com/grpc/grpc-go/blob/ec9275ba9aded9f4dade1cde439fdb3f40d98d03/stream.go#L297-L304

The main purpose of this goroutine is to monitor the user's context in order to abort the RPC when they cancel it. This introduces some moderate overhead on the cost of an RPC (~1-2%). To avoid this, we could instead add a Cancel() to clientStream that users are able to call synchronously when they wish to cancel the stream (we may be doing this anyway). With this in place, if the user agrees to use it instead of the context for unscheduled cancellations (i.e. besides deadlines), we no longer need to fork this goroutine. There are some other prerequisites for this work. To list them all:

  1. Implement clientStream.Cancel() (#1933)
  2. Use a different mechanism to cancel streams when the ClientConn is closed. E.g. have ClientConn.Close() synchronously cancel all active streams, or use callbacks from the transport when the net.Conn is closed.
  3. Use a different mechanism to cancel streams when the deadline is reached. This could be one goroutine per ClientConn that sleeps until the next deadline, or until it is awoken due to a new deadline or one being removed. Since Go doesn't support WaitUntil on a sync.Cond (https://github.com/golang/go/issues/24429), something else will have to be used (e.g. a time.Timer, a channel, and blocking on a select). [EDIT: or use a sync.Cond and do time.AfterFunc(nextTime, cond.Broadcast); stopping that timer if awoken before it fires.]

(It's possible 3 has similar overhead to forking a goroutine, in which case this would not be worth pursuing, so we would have to measure performance before starting on this.)

dfawley avatar Mar 16 '18 17:03 dfawley

I just recently ran into this- I'm curious if any code today would break if CloseSend actually "Closed". I've only seen CloseSend in defer statements in all example code, so maybe having CloseSend cause this goroutine to exit would be safe?

cstockton avatar Apr 20 '18 01:04 cstockton

@cstockton Are you thinking of CloseSend on the client or the server? This bug covers client-side; I don't think you could finish the clientStream or abort that goroutine when CloseSend is called, since the RPC is still active and closing send to signal to the server that the client is done is a common use case. On the server, this goroutine doesn't exist, so nothing there is necessary.

dfawley avatar Apr 20 '18 15:04 dfawley

I've only seen CloseSend in defer statements in all example code

Can you point me to the example you are talking about here? Calling CloseSend in defer doesn't sound right to me. And I believe we don't have any defer CloseSend in this repo.

menghanl avatar Apr 20 '18 17:04 menghanl

@dfawley That is exactly what I was thinking, because CloseSend is on "Recv" only API's and it was also the only "Close" related function I could find. I posted my closing thoughts https://github.com/grpc/grpc-go/issues/2015 there. What it boils down to is as a client I want to be certain of one thing when I exit my call frame: request resources are released. I guess that just isn't possible without receiving all items, which I feel is unacceptable but I don't expect everyone will agree.

cstockton avatar Apr 20 '18 19:04 cstockton