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

Need a way to stop server side streaming when client disconnected #6456

Closed
pfwang80s opened this issue Nov 21, 2019 · 10 comments
Closed

Need a way to stop server side streaming when client disconnected #6456

pfwang80s opened this issue Nov 21, 2019 · 10 comments
Labels

Comments

@pfwang80s
Copy link

pfwang80s commented Nov 21, 2019

here is pseudo code to impl a server side streaming:

@Override
public void someMethod(XXRequest request,  StreamObserver<XXEvent> responseObserver) {
	try {
	            while (!Context.current().isCancelled()) {
	                XXEvent event = heavyCompute();
	                if (event != null) {
	                    responseObserver.onNext(event);
	                }
	            }
	} finally {
	       doClean();
	}
}

On client side, if everything is ok, CancellableContext.cancel() or ManagedChannel.shutdownNow() will terminate someMethod call gracefully, but if client crashes && heavyCompute always returns null, someMethod call will run forever...

so is there a way to fix this?

Thanks.

@sanjaypujare
Copy link
Contributor

Couple of ways:

  • you need another way to break out of your while loop (either number of iterations or a timer)
  • how about using a CancellableContext with a deadline in your while loop? Look at the Javadoc of io.grpc.Context.withDeadline for sample usage.

@pfwang80s
Copy link
Author

Couple of ways:

  • you need another way to break out of your while loop (either number of iterations or a timer)
  • how about using a CancellableContext with a deadline in your while loop? Look at the Javadoc of io.grpc.Context.withDeadline for sample usage.

Thank you for your reply. In my logic, the server returns an infinite event stream that a healthy client consumes event from, so withDeadline may not work, did I misunderstand it?

A workaround is change someMethod to bi-stream, and treat client stream as Ping, but it looks not good...

@sanjaypujare
Copy link
Contributor

If the client has crashed and your event is always null then you go into an infinite loop because of the structure of your while loop. When event is null you can check the state of the responseObserver by casting to ServerCallStreamObserver and calling isCancelled assuming that works when client has crashed. Or you can limit the number of iterations you will perform when event is consecutively null.

@voidzcy
Copy link
Contributor

voidzcy commented Nov 21, 2019

What do you mean by "crashes"? If the client process closes the socket after it "crashes", the stream will be closed and server RPC context will be cancelled. What you are currently checking in your while loop can handle it.

@pfwang80s
Copy link
Author

If the client has crashed and your event is always null then you go into an infinite loop because of the structure of your while loop. When event is null you can check the state of the responseObserver by casting to ServerCallStreamObserver and calling isCancelled assuming that works when client has crashed. Or you can limit the number of iterations you will perform when event is consecutively null.

Thank you. most of heavyCompute() call returns null, no matter if client gone, I just want to quit as soon as possible to save resources.

@pfwang80s
Copy link
Author

What do you mean by "crashes"? If the client process closes the socket after it "crashes", the stream will be closed and server RPC context will be cancelled. What you are currently checking in your while loop can handle it.

"crashes" means that client has no chance doing any clean operation, e.g.: network connections broken, or a segmentation exception.

Here is a simple logic with pseudo code:
server.start();
channel := new channel(svr_endpoint);
client := newStub(channel);
iterator := client.someMethod(request);
iterator.nextSomeTimes();
channel.showdown();
Thread.sleep(infinite);

I've waited for someMethod at server-side for 100 seconds, but it didn't terminated as Context.current().isCancelled() returned false.
I can't understand when channel.shutdown(), why Context.current().isCanceled() was returning false continuously for such a long time.

@zhangkun83
Copy link
Contributor

If client or network failed so badly that it has not chance notifying the server side, the TCP connection could stall for very long time until it times out. You can try setting keepAliveTime() in NettyServerBuilder to a smaller number (default 2 hours). Combined with keepAliveTimeout() (default 20s), that would make the server wait at most keepAliveTime + keepAliveTimeout to notice the bad connection.

I can't understand when channel.shutdown(), why Context.current().isCanceled() was returning false continuously for such a long time.

I thought you were complaining the case when client crashes, not when the client calls channel.shutdown() normally.

@pfwang80s
Copy link
Author

Thank you for your reply, but I found NettyServerBuilder was introduced at 2016, until now, it has being an experimental feature. Do we plan to turn it normal?

If client or network failed so badly that it has not chance notifying the server side, the TCP connection could stall for very long time until it times out. You can try setting keepAliveTime() in NettyServerBuilder to a smaller number (default 2 hours). Combined with keepAliveTimeout() (default 20s), that would make the server wait at most keepAliveTime + keepAliveTimeout to notice the bad connection.

I can't understand when channel.shutdown(), why Context.current().isCanceled() was returning false continuously for such a long time.

I thought you were complaining the case when client crashes, not when the client calls channel.shutdown() normally.

@voidzcy
Copy link
Contributor

voidzcy commented Nov 25, 2019

NettyServerBuilder is pretty stable, but it depends on Netty's feature, we reserve the right to change its interface if necessary (e.g., some breaking changes in Netty). In general, we will avoid such a change as much as possible.

@dapengzhang0
Copy link
Member

@pengfeiwang-cn If you are developing libraries that other people are depending on, do not use ExperimentalAPI; if you are developing applications, the keepalive API in NettyServerBuilder is pretty safe to use in production.

@dapengzhang0 dapengzhang0 added this to the Unscheduled milestone Jan 6, 2020
@ejona86 ejona86 removed this from the Unscheduled milestone Mar 26, 2020
@ejona86 ejona86 closed this as completed Mar 27, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants