Neither OnCancelHandler nor onComplete gets invoked after half-close #5882
Comments
|
The server calling OnCancelHandler isn't called because there wasn't a cancellation; the RPC completed successfully. onComplete isn't called because the client didn't halfClose() (the client didn't call onComplete() before the server finished the RPC). Client-side looks to be operating correctly. On server-side I expect the ServerCall is working correctly. This seems like a problem in the stub. I think we could fabricate an onError when the RPC terminates if we haven't yet called onError/onCompleted. That'd happen in ServerCalls. I think this would be easy to fix.
No, OnCancelHandler isn't appropriate for that. On the lower-level one of onCancel (same as the handler)/onComplete (different than StreamObserver.onCompleted) is guaranteed to be called. But we don't expose onComplete today. We could add that though, I think. |
|
Re half-close - is this the same in other GRPC implementations? I was trying to find the documentation describing half-closed connections and how they should/ are working, but haven't found anything. The initial expectation was that in bidirectional streaming case, streams are completely independent and either party can and should signal stream end (completion) and there should be no distinction between client and server. Is there some specific reason why onCompleted event issued by the server results in call termination rather than half-closed stream? Re OnCancelHandler, actually my expectation was based on ServerCallStreamObserver.setOnCancelHandler(..) javaDoc which says: "Set a Runnable that will be called if the calls isCancelled() state changes from false to true.". Actual implementation seems to be contradicting to that, since if we launch a background thread which periodicly checks streamObserver.isCancelled() eventually it does return true, but provided OnCancelHandler doesn't get called. |
|
The best documentation for the behavior is probably in ServerCall/ClientCall (either one; they should mirror each other) or in grpc/grpc#15460 . The ServerCall/ClientCall APIs were designed to be close to 1:1 with the underlying protocol, so they match the cross-language restrictions pretty well. Yes, that PR linked isn't merged, although it is mainly for the word choice of what we call various concepts, not for details about the behavior. It is unfortunate that the cross-language documentation isn't clearly specified, but that only really becomes a problem for bidi streaming. And it shouldn't hurt Java users much since ServerCall/ClientCall defines quite a bit. Granted, you should have been able to just read the various StreamObserver documentation. You shouldn't need to read ServerCall/ClientCall, but that would have probably gone better we had exposed ServerCall.Listener.onCompleted() to StreamObserver API. Although the conflicting names add confusion. https://grpc.github.io/grpc-java/javadoc/io/grpc/ClientCall.Listener.html#onClose-io.grpc.Status-io.grpc.Metadata- |
|
This issue will be for the bug of not calling onError (since the StreamObserver API expects to always end in onError/onComplete). I've created #5895 for the RPC's completion notification feature. |
|
Eric, if StreamObserver will always end with onError/onComplete invocation, I think that will be sufficient and another onComplete handler (to deallocate server resources) is just a nice to have feature. In addition, as you mentioned, there might be a naming clash, which most likely will be hard to understand for average user. In addition to that I still think that there is a gap between ServerCallStreamObserver.isCancelled() and ServerCallStreamObserver.setOnCancelHandler(...) - at least in javaDocs, which state that handler will be invoked whenever isCancelled() switches from false to true. If you'll start a background thread on server which periodically checks isCancelled() value, you'll notice that eventually it switches from false to true, even if call has been completed gracefully, but it looks like that this handler gets only invoked if call has been terminated without a half-close. Notice that I am reffering to ServerCallStreamObserver.isCancelled() javaDocs not the ServerCall.isCancelled(), which is a bit different. Example Client: |
What... grpc-java/core/src/main/java/io/grpc/internal/ServerCallImpl.java Lines 349 to 354 in e57d4c5 But I tried your program and confirmed the behavior. I tracked the problem to the Context (correctly) being cancelled but then that sets cancelled = true. grpc-java/core/src/main/java/io/grpc/internal/ServerCallImpl.java Lines 278 to 285 in e57d4c5 That behavior's totally a bug. It appears introduced in #2963 which was version 1.5.0. I tested 1.4.0 and isCancelled never swapped to true. Fixing the bug seems likely to break users (unknowingly) depending on the current behavior. I may have to redefine isCancelled to say it will return true if the call is cancelled or when it is complete. Alternatively I could try fixing the bug internally and seeing how much breaks; if not very much, then maybe there aren't many users impacted. |
I didn't receive a notification in either case; half-close didn't seem to change whether the cancellation handler was called. That's the behavior I would expect. |
|
Do you think this is the same reason which cause my issue? #6011 |
|
Is there any update on this? I have an application which depends on |
The bug is actually in grpc and not our code, described here: grpc/grpc-java#5882 using bidirectional streaming we can not invoke "onCompleted" twice (needed by client & server), Note: An exception to this is calling onCompleted from inside onCompleted (this works and this is the temp fix for "one ride reserver", however in our user case this is not relevant as we need to open async requests) Our bug: Once the server finds a ride and trigger the client's onComplete, the client can not trigger back onComplete for the server, which leads to the semaphore in the server to not to be released, we should think of other way implementing this. One idea is to not relay on the "onCompleted" and use the actual streaming to sends messages between the client and server, thus the end of the stream can be known when we get a specific message. (via onNext()) If you want to do this, change the grpc method signature, to accept "RequestHeader" (implement it in protobuf), which contains a string message, and data, same for ResponseHeader,
I seem to be still observing this on Has there been any decisions made on whether this is going to be fixed or is going to stay the way it is (requiring workarounds)? If this is going to be fixed, but needs time, given pointers I may be able to contribute, otherwise, what's the recommended workaround that'd allow resource cleanup, much like the transition to |
|
This issue is just for the problem on server-side where
If you want to be notified on server-side when the RPC is complete and not have to rely on cleaning up yourself when your server calls onError/onComplete, then that would be #5895 that I split out of this issue. An approach available today would be to add a |
|
@mohsenrezaeithe, I'd suggest we discuss that on #5895, if that is the piece you are wanting. That feature is very easy to implement. I think we'd mainly argue over what to call the method name. |
…d. (#8408) The semantics around cancel vary slightly between ServerCall and CancellableContext - the context should always be cancelled regardless of the outcome of the call while the ServerCall should only be cancelled on a non-OK status. This fixes a bug where the ServerCall was always marked cancelled regardless of call status. Fixes #5882
|
API Review meeting notes (from Friday, 8/20/2021; just slow copying the notes here):
|
What version of gRPC are you using?
1.21.0 (same applies to older versions as well)
Issue
1
After server sends completion event to the client, i.e., half-closes the call, client doesn't dispatch any other events (although request stream isn't closed/ completed).
2
In addition to that server OnCancelHandler doesn't get ever invoked, although client shuts down and closes connection.
Server:
Client:
Client logs:
Server logs:
What did you expect to see?
1
Client should keep on sending events to server until it closes/ completes request stream, independently if server closed response stream (half-closed call). As of now client is not able to reply to server upon response stream completion.
2
Servers OnCancelHandler must be invoked in either case, independently if call has been already half-closed or not and if connection has been forcibly shutdown or not. It should be possible to free-up resources like database connections in OnCancelHandler, but as off now it doesn't seem that there is a single place where we could do this.
The text was updated successfully, but these errors were encountered: