improve: RpcInvokeCallbackListener resolve response #336
Conversation
src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java
Outdated
Show resolved
Hide resolved
WalkthroughThe recent changes encompass a variety of improvements and additions across multiple files, focusing on enhancing callback handling, updating dependencies, refining connection status checks, and adding new test cases. Key modifications include refactoring the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant RpcCommandHandler
participant RemotingContext
participant ProcessMethod
User->>RpcCommandHandler: Invoke run()
RpcCommandHandler->>RemotingContext: Clone ctx
RpcCommandHandler->>ProcessMethod: Call process(ctx.clone(), m)
ProcessMethod-->>RpcCommandHandler: Return result
RpcCommandHandler-->>User: Return response
Poem
Warning Review ran into problemsProblems (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java (1 hunks)
Additional comments not posted (1)
src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java (1)
136-147: Ensure thread safety when modifying the response object.synchronized (response) { Object responseObj = RpcResponseResolver.resolveResponseObject(response, this.remoteAddress); response.setInvokeContext(future.getInvokeContext()); callback.onResponse(responseObj); }This modification ensures that changes to the response object are thread-safe, which is crucial in a multi-threaded environment.
src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java
Outdated
Show resolved
Hide resolved
JoeCqupt
force-pushed
the
resolve-response
branch
from
206f2e4
to
f6dd49e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java
JoeCqupt
force-pushed
the
resolve-response
branch
from
a22ee85
to
8187102
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java
|
It's amazing, I feel like this CI will randomly fail |
JoeCqupt
force-pushed
the
resolve-response
branch
from
c84504c
to
8187102
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- .github/workflows/release.yml (1 hunks)
- pom.xml (5 hunks)
- src/main/java/com/alipay/remoting/BaseRemoting.java (2 hunks)
- src/main/java/com/alipay/remoting/Connection.java (1 hunks)
- src/main/java/com/alipay/remoting/InvokeContext.java (3 hunks)
- src/main/java/com/alipay/remoting/RemotingContext.java (3 hunks)
- src/main/java/com/alipay/remoting/rpc/protocol/RpcCommandHandler.java (1 hunks)
- src/main/java/com/alipay/remoting/rpc/protocol/RpcRequestProcessor.java (4 hunks)
- src/main/java/com/alipay/remoting/serialization/SerializerManager.java (2 hunks)
- src/test/java/com/alipay/remoting/inner/connection/ConnectionTest.java (2 hunks)
- src/test/java/com/alipay/remoting/rpc/exception/ExceptionTest.java (1 hunks)
- src/test/java/com/alipay/remoting/rpc/protocol/RpcCommandHandlerTest.java (1 hunks)
- src/test/java/com/alipay/remoting/rpc/serializer/ClassCustomSerializerTest.java (3 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/release.yml
- pom.xml
Additional comments not posted (20)
src/main/java/com/alipay/remoting/serialization/SerializerManager.java (2)
19-19: Import and declaration ofReentrantLockare appropriate for ensuring thread safety.Also applies to: 37-37
40-53: Thread safety improvements ingetSerializermethod are correctly implemented usingReentrantLock.src/test/java/com/alipay/remoting/rpc/protocol/RpcCommandHandlerTest.java (2)
55-71: The new test methodtestHandleCommandis well-structured and correctly tests command handling with asynchronous behavior.
73-143: TheMockUserProcessorsclass is correctly implemented and serves its purpose in the test setup.src/test/java/com/alipay/remoting/inner/connection/ConnectionTest.java (1)
128-133: The new test methodtestIsFinecorrectly tests the connection's fine-check logic after closing the connection.src/main/java/com/alipay/remoting/InvokeContext.java (3)
76-85: Addition ofRecordContextfield and its initialization in the constructor are correctly implemented to enhance functionality.
137-137: Changes to theclearmethod, including resetting theRecordContext, are appropriate for ensuring consistency in the state ofInvokeContext.
146-148: ThegetRecordContextmethod is correctly implemented to provide access to theRecordContextfield.src/main/java/com/alipay/remoting/RemotingContext.java (2)
121-131: The addition ofwriteandflushmethods inRemotingContextcorrectly abstracts lower-level channel operations, enhancing the usability of the class.
260-262: Theclonemethod inRemotingContextis correctly implemented to provide a shallow copy of the context, suitable for its intended use.src/main/java/com/alipay/remoting/rpc/protocol/RpcCommandHandler.java (1)
108-108: Cloning thectxobject inRpcCommandHandlerbefore processing commands is a crucial change for ensuring that the original context remains unaltered, enhancing thread safety.src/test/java/com/alipay/remoting/rpc/exception/ExceptionTest.java (1)
345-365: The new test methodtestGetBizClassLoaderException1effectively tests the exception handling whengetBizClassLoaderthrows aRuntimeException. This is a good addition to ensure robustness in error scenarios.src/main/java/com/alipay/remoting/Connection.java (1)
160-160: The addition of the!closed.get()condition in theisFinemethod is a crucial improvement. It ensures that the connection's closed state is checked along with its active state, which is essential for accurate connection health checks.src/main/java/com/alipay/remoting/BaseRemoting.java (1)
44-47: The change from a private to a protectedcommandFactoryand its initialization in the constructor enhances the flexibility and extensibility of theBaseRemotingclass. This allows subclasses to directly access and utilize thecommandFactory, which is a good design decision for future enhancements and customizations.src/main/java/com/alipay/remoting/rpc/protocol/RpcRequestProcessor.java (4)
23-25: The addition of imports forRecordContext,RecordScene, andRecorderManagerindicates an enhancement in the monitoring or logging capabilities of theRpcRequestProcessor. This is a positive change as it likely supports more detailed or context-aware logging or metrics collection.
137-156: The introduction of recording logic usingRecorderManagerin thedoProcessmethod is a significant enhancement. It allows for detailed tracking and analysis of request handling, which can be crucial for performance tuning and debugging. The use oftry-finallyensures that recording stops even if an exception occurs, which is a good practice for maintaining accurate metrics.
193-198: The nested try-catch block inside the catch for serialization exceptions ensures that even if serialization of the exception response fails, the error is logged. This is a robust error handling strategy that prevents loss of error information, crucial for diagnosing issues in production environments.
415-419: The detailed error logging and response handling in theProcessTaskclass'srunmethod is well-implemented. It ensures that any exceptions during the processing of RPC requests are not only logged with detailed context but also communicated back to the caller, enhancing the reliability and debuggability of the system.src/test/java/com/alipay/remoting/rpc/serializer/ClassCustomSerializerTest.java (2)
23-23: Added imports forAtomicBoolean,DefaultCustomSerializer,ResponseCommand,InvokeServerException, andRpcServerException.These imports are necessary for the new test method
testResponseSerialThrowableto function correctly.Also applies to: 25-28
309-346: New test methodtestResponseSerialThrowableadded to handle serialization exceptions for response objects.This test method effectively checks the serialization process by intentionally throwing a
StackOverflowErrorto simulate an error scenario. It's a good practice to include such robustness tests. However, ensure that theStackOverflowErroris a deliberate choice for simulating serialization errors, as it typically indicates very different issues (such as infinite recursion).
JoeCqupt
force-pushed
the
resolve-response
branch
from
8187102
to
9f4499e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/alipay/remoting/rpc/RpcInvokeCallbackListener.java
|
这个CI有问题，线下跑的单测没问题，CI跑的就报错 线下： CI： |
Why make this change?
The code for resolve response is duplicated。
replace duplicated code by using
RpcResponseResolver.resolveResponseObjectmethodSummary by CodeRabbit
New Features
RemotingContextinstances.Bug Fixes
isFine()method inConnectionto properly check the closed status.Improvements
CallbackTaskclass to enhance exception handling, error messages, and callback invocation logic.pom.xmlfor improved stability and performance.BaseRemotingfor better clarity.SerializerManager.Tests