Skip to content

api: Implement custom events framework in gRPC-Java server - #12980

Open
kannanjgithub wants to merge 18 commits into
grpc:masterfrom
kannanjgithub:server-framework-custom-events
Open

api: Implement custom events framework in gRPC-Java server#12980
kannanjgithub wants to merge 18 commits into
grpc:masterfrom
kannanjgithub:server-framework-custom-events

Conversation

@kannanjgithub

Copy link
Copy Markdown
Contributor

This adds triggerEvent/onEvent APIs to ServerCall and ServerCall.Listener routing them through ServerStream transport.

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

Copy link
Copy Markdown
Contributor Author

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

@sauravzg sauravzg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the source.

Comment thread api/src/main/java/io/grpc/ServerCall.java
Comment thread api/src/main/java/io/grpc/ServerCall.java
Comment thread core/src/main/java/io/grpc/internal/ServerCallImpl.java
Comment thread core/src/main/java/io/grpc/internal/ServerCallImpl.java Outdated
Comment thread api/src/main/java/io/grpc/ServerCall.java
Comment thread core/src/main/java/io/grpc/internal/ServerCallImpl.java
Comment thread binder/src/test/java/io/grpc/binder/AsyncSecurityPoliciesTest.java
Comment thread binder/src/main/java/io/grpc/binder/internal/Inbound.java Outdated
…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 sauravzg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have enough test coverage? I see some of source files with changes but not their corresponding test files?

Comment thread binder/src/test/java/io/grpc/binder/AsyncSecurityPoliciesTest.java
Comment thread api/src/main/java/io/grpc/ServerCall.java
Comment thread core/src/main/java/io/grpc/internal/ServerCallImpl.java
…d is true

Check closeCalled before dispatching triggerEvent to the transport
stream, avoiding unnecessary task allocations and transport hops if
the call has already been closed.

TAG=agy
CONV=e1bfa5a2-e855-4f79-abdd-ef2b264977be
…stom event changes

- Test onEvent throwing StatusRuntimeException closes the call with status and trailers.
- Test onEvent throwing StatusRuntimeException on an already closed call does not trigger duplicate close.
- Test SerializingServerCall executes triggerEvent sequentially in FIFO order on serializingExecutor.

TAG=agy
CONV=e1bfa5a2-e855-4f79-abdd-ef2b264977be
@kannanjgithub

Copy link
Copy Markdown
Contributor Author

Do we have enough test coverage? I see some of source files with changes but not their corresponding test files?

Only the tests for the changes in TransmitStatusRuntimeExceptionInterceptor were missed, added that now.

…e stream cleanup in AbstractTransportTest

In MockServerTransportListener.streamCreated(), stream.setListener(listener)
was called after streams.add(StreamCreation(...)). This created a race
condition where a test thread calling takeStreamOrFail() could dequeue
the stream and call serverStream.triggerEvent() before stream.setListener()
was called by the transport/container thread. When this occurred (e.g. in
TomcatTransportTest on multi-core runners), ServletServerStream invoked
transportState.triggerEvent() on the test thread, saw a null listener,
threw a NullPointerException (swallowed by SerializingExecutor), and
never enqueued the event into the listener queue, leading to a timeout
and assertion failure:
    expected:<...Object@...> but was:<null>

Setting stream.setListener(listener) before enqueuing to streams guarantees
that any thread consuming the StreamCreation will always observe a fully
initialized listener.

Additionally, in AbstractTransportTest.serverStream_triggerEvent(), replace
clientStream.cancel(Status.CANCELLED) with serverStream.close(Status.OK, ...)
for clean stream closure instead of leaving an uncoordinated client RST_STREAM
in flight during container tearDown.

TAG=agy
CONV=e1bfa5a2-e855-4f79-abdd-ef2b264977be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants