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.

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

New CallOption: grpc.ManualCancel #1923

Open
dfawley opened this issue Mar 16, 2018 · 4 comments
Open

New CallOption: grpc.ManualCancel #1923

dfawley opened this issue Mar 16, 2018 · 4 comments
Labels
Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. P3 Type: Performance Performance improvements (CPU, network, memory, etc)

Comments

@dfawley
Copy link
Member

dfawley commented Mar 16, 2018

We fork a goroutine for every stream here:

grpc-go/stream.go

Lines 297 to 304 in ec9275b

go func() {
select {
case <-cc.ctx.Done():
cs.finish(ErrClientConnClosing)
case <-ctx.Done():
cs.finish(toRPCErr(ctx.Err()))
}
}()

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() (New API: grpc.CancelClientStream(ClientStream) #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 (sync: Cond WaitFor and/or WaitUntil method(s) golang/go#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 dfawley added P2 Type: Performance Performance improvements (CPU, network, memory, etc) labels Mar 16, 2018
@cstockton
Copy link

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?

@dfawley
Copy link
Member Author

dfawley commented Apr 20, 2018

@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.

@menghanl
Copy link
Contributor

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.

@cstockton
Copy link

@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 #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.

@stale stale bot added the stale label Sep 6, 2019
@dfawley dfawley removed the stale label Sep 6, 2019
@grpc grpc deleted a comment from stale bot May 4, 2021
@menghanl menghanl added P3 and removed P2 labels May 4, 2021
@eshitachandwani eshitachandwani added the Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Client Includes Channel/Subchannel/Streams, Connectivity States, RPC Retries, Dial/Call Options and more. P3 Type: Performance Performance improvements (CPU, network, memory, etc)
Projects
None yet
Development

No branches or pull requests

5 participants