fix(AgentBase): Optimize AgentBase's lock management to resolve concurrency conflicts.#733
Conversation
…ource management Two new test methods were added to verify the agent's behavior under concurrent invocation, and resource management and exception handling mechanisms in the `AgentBase` class were improved. Related dependencies and import statements were also updated.
Summary of ChangesHello @wuji1428, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical concurrency conflict within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a concurrency issue in AgentBase by replacing doFinally with an eager Mono.using. This change correctly ensures that the agent's running state is reset before the operation completes, preventing race conditions. The added tests in AgentBaseTest and ReActAgentStructuredOutputTest validate this fix for different call method overloads. I've included one suggestion to refactor the call methods in AgentBase to reduce code duplication and improve maintainability.
| return Mono.using( | ||
| () -> { | ||
| if (checkRunning && !running.compareAndSet(false, true)) { | ||
| throw new IllegalStateException( | ||
| "Agent is still running, please wait for it to finish"); | ||
| } | ||
| resetInterruptFlag(); | ||
| return this; | ||
| }, | ||
| resource -> | ||
| TracerRegistry.get() | ||
| .callAgent( | ||
| this, | ||
| msgs, | ||
| () -> | ||
| notifyPreCall(msgs) | ||
| .flatMap(this::doCall) | ||
| .flatMap(this::notifyPostCall) | ||
| .onErrorResume( | ||
| createErrorHandler( | ||
| msgs.toArray(new Msg[0])))), | ||
| resource -> running.set(false), | ||
| true); |
There was a problem hiding this comment.
The three overloaded call methods (call(List<Msg>), call(List<Msg>, Class<?>), and call(List<Msg>, JsonNode)) share nearly identical logic for handling the agent's running state using Mono.using. This code duplication can be eliminated by extracting the common logic into a private helper method. This would improve maintainability, as future changes to the lock management would only need to be made in one place.
Here is a suggested refactoring that would replace all three call methods and add a new private executeCall method:
@Override
public final Mono<Msg> call(List<Msg> msgs) {
return executeCall(msgs, this::doCall);
}
@Override
public final Mono<Msg> call(List<Msg> msgs, Class<?> structuredOutputClass) {
return executeCall(msgs, m -> doCall(m, structuredOutputClass));
}
@Override
public final Mono<Msg> call(List<Msg> msgs, JsonNode schema) {
return executeCall(msgs, m -> doCall(m, schema));
}
private Mono<Msg> executeCall(List<Msg> msgs, Function<List<Msg>, Mono<Msg>> doCallLogic) {
return Mono.using(
() -> {
if (checkRunning && !running.compareAndSet(false, true)) {
throw new IllegalStateException(
"Agent is still running, please wait for it to finish");
}
resetInterruptFlag();
return this;
},
resource ->
TracerRegistry.get()
.callAgent(
this,
msgs,
() ->
notifyPreCall(msgs)
.flatMap(doCallLogic)
.flatMap(this::notifyPostCall)
.onErrorResume(
createErrorHandler(
msgs.toArray(new Msg[0])))),
resource -> running.set(false),
true);
}Since a single code suggestion cannot span all three methods across different parts of the file, I'm providing the full refactoring here for your consideration.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
AgentScope-Java Version
[The version of AgentScope-Java you are working on, e.g. 1.0.8, check your pom.xml dependency version or run
mvn dependency:tree | grep agentscope-parent:pom(only mac/linux)]Description
Ref Issue #732
Replace the doFinally structure with Mono.using and set the eager parameter to true. This ensures that the state bit is forcibly reset before the onComplete or onError signals reach the downstream subscriber, thereby eliminating the race condition.
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)