api: Implement custom events framework in gRPC-Java server - #12980
api: Implement custom events framework in gRPC-Java server#12980kannanjgithub wants to merge 15 commits into
Conversation
This adds triggerEvent/onEvent APIs to ServerCall and ServerCall.Listener, routing them through ServerStream transport to ensure thread-safety (especially for SerializeReentrantCallsDirectExecutor). TAG=agy CONV=e1bfa5a2-e855-4f79-abdd-ef2b264977be
|
Need to implement methods in Binder transport. |
…framework. - Added unit tests in AbstractServerStreamTest for triggerEvent propagation and close behavior. - Updated ContextsTest to cover onEvent propagation in ContextualizedServerCallListener. TAG=agy CONV=e1bfa5a2-e855-4f79-abdd-ef2b264977be
Synchronized with the executor before asserting cancellation of the delegate future to ensure that transformAsync has finished processing the delegate future and propagated the cancellation. TAG=agy CONV=e1bfa5a2-e855-4f79-abdd-ef2b264977be
… behavior. - Added unit tests in ServerImplTest for JumpToApplicationThreadServerStreamListener.triggerEvent. - Added serverStream_triggerEvent_afterClose in AbstractTransportTest to verify events are ignored after stream closure. - Updated Inbound.ServerInbound to check isClosed() before triggering events. TAG=agy CONV=e1bfa5a2-e855-4f79-abdd-ef2b264977be
Wait for the server stream to be fully closed (via awaitClose) before calling triggerEvent, to ensure the transport has processed the cancellation and marked the listener as closed. This fixes flakiness in slower transports like Jetty. TAG=agy CONV=e1bfa5a2-e855-4f79-abdd-ef2b264977be
Updated ServerInbound.triggerEvent to invoke the listener's triggerEvent callback inside the synchronized(this) block. This ensures that the check for isClosed() and the invocation of the listener are atomic relative to stream closure (which also runs under the same lock). This prevents a race where triggerEvent could be called on the listener after the stream has been closed, which would result in out-of-order events delivered to the application. This is consistent with how other listener callbacks (like closed and halfClosed) are delivered in Inbound.java. TAG=agy CONV=e1bfa5a2-e855-4f79-abdd-ef2b264977be
| * @param event the triggered event. | ||
| */ | ||
| @ExperimentalApi("https://github.com/grpc/grpc-java/issues/12979") | ||
| public void onEvent(Object event) { |
There was a problem hiding this comment.
Any invariants on this ? Can this be called after cancellation(I assume no)?
There was a problem hiding this comment.
The framework code (ServerStreamListenerImpl) calls onEvent and it will drop it if the call is cancelled.
There was a problem hiding this comment.
The expected behavior should be documented on the interface on if it's callable after cancellation/completion , if yes, what's the expected behavior.
| } | ||
|
|
||
| @Override | ||
| public void triggerEvent(Object event) { |
There was a problem hiding this comment.
do we need a cancellation and close check here? other methods seem to have it.
closeCalled is interesting because it'd require us to make it volatile which may break other assumptions about thread safety.
There was a problem hiding this comment.
A check in ServerCallImpl could never prevent concurrent races anyway. Because ServerCall.triggerEvent is intended to be thread-safe, external threads may invoke call.triggerEvent(...) concurrently with the application thread invoking call.close(...).
- Even if
closeCalledwerevolatile, thread A could readcloseCalled == falsea nanosecond before thread B setscloseCalled = true. - An event could therefore always enter
stream.triggerEvent(...)whilecloseis in progress. - Thus, the authoritative synchronization point must reside in the transport layer where stream lifecycle state and inbound events are serialize.
The listenerClosed check in AbstractServerStream.triggerEvent completely compensates for not checking closeCalled in ServerCallImpl.triggerEvent, by dropping the event if the listener is closed.
There was a problem hiding this comment.
I am less worried about races since we can fix and address them and more worried about the expected behavior.
Right now it seems like we don't check cancellation or closure status here when triggering operations which may be okay if our interface contract is "you should not call after cancellation" . If our contract allows or specifies the behavior after cancellation , I'd assume we check and enforce it here in the implementation.
| ListenableFuture<Status> authFuture = asyncPolicy.checkAuthorizationAsync(SOME_UID); | ||
| assertThat(awaitResult(settableUid)).isEqualTo(SOME_UID); | ||
| authFuture.cancel(false); | ||
| executor.submit(() -> {}).get(10, TimeUnit.SECONDS); |
There was a problem hiding this comment.
what are we doing here? Seems like no-op to me.
There was a problem hiding this comment.
It is causing a wait for the cancellation task submitted to the executor to be complete before the assertion in the next statement.
There was a problem hiding this comment.
and why did we need it now and not before this PR? or is this an unrelated change?
…ExceptionInterceptor, and OpenTelemetryTracingModule. - binder: Implement onEvent in PendingAuthListener to buffer and replay custom events to the delegate once auth completes, preventing events from being dropped. - util: Handle onEvent in TransmitStatusRuntimeExceptionInterceptor listener wrapper to catch StatusRuntimeException and close the call. Serialize triggerEvent on SerializingServerCall's executor. - opentelemetry: Implement onEvent in ContextServerCallListener to attach OpenTelemetry trace context and scope during delegate invocation. - Add unit tests for all updated implementations. TAG=agy CONV=e1bfa5a2-e855-4f79-abdd-ef2b264977be
…mListenerImpl triggerEvent Wrap ServerCallImpl.triggerEvent and ServerStreamListenerImpl.triggerEvent in PerfMark.traceTask with PerfMark.attachTag, aligning them with sendMessage, sendHeaders, close, request, and listener callbacks. TAG=agy CONV=e1bfa5a2-e855-4f79-abdd-ef2b264977be
sauravzg
left a comment
There was a problem hiding this comment.
Do we have enough test coverage? I see some of source files with changes but not their corresponding test files?
| ListenableFuture<Status> authFuture = asyncPolicy.checkAuthorizationAsync(SOME_UID); | ||
| assertThat(awaitResult(settableUid)).isEqualTo(SOME_UID); | ||
| authFuture.cancel(false); | ||
| executor.submit(() -> {}).get(10, TimeUnit.SECONDS); |
There was a problem hiding this comment.
and why did we need it now and not before this PR? or is this an unrelated change?
| * @param event the triggered event. | ||
| */ | ||
| @ExperimentalApi("https://github.com/grpc/grpc-java/issues/12979") | ||
| public void onEvent(Object event) { |
There was a problem hiding this comment.
The expected behavior should be documented on the interface on if it's callable after cancellation/completion , if yes, what's the expected behavior.
| } | ||
|
|
||
| @Override | ||
| public void triggerEvent(Object event) { |
There was a problem hiding this comment.
I am less worried about races since we can fix and address them and more worried about the expected behavior.
Right now it seems like we don't check cancellation or closure status here when triggering operations which may be okay if our interface contract is "you should not call after cancellation" . If our contract allows or specifies the behavior after cancellation , I'd assume we check and enforce it here in the implementation.
This adds triggerEvent/onEvent APIs to
ServerCallandServerCall.Listenerrouting them throughServerStreamtransport.