Skip to content

refactor(core): add timeouts to block() and use Objects utilities#836

Open
no-0-name wants to merge 1 commit intoagentscope-ai:mainfrom
no-0-name:refactor/core-block-timeouts-and-objects-utils
Open

refactor(core): add timeouts to block() and use Objects utilities#836
no-0-name wants to merge 1 commit intoagentscope-ai:mainfrom
no-0-name:refactor/core-block-timeouts-and-objects-utils

Conversation

@no-0-name
Copy link

Description

Adds explicit timeouts to all block() calls in core to avoid indefinite blocking, and uses Objects.requireNonNull / Objects.requireNonNullElse where appropriate for consistency with project style.

Changes:

  • KnowledgeRetrievalTools: Constants DEFAULT_RETRIEVE_LIMIT (5) and RETRIEVE_TIMEOUT (60s); Objects.requireNonNull(knowledge) in constructor; Objects.requireNonNullElse(limit, DEFAULT_RETRIEVE_LIMIT); block(RETRIEVE_TIMEOUT) for retrieval.
  • MsgHub: CLOSE_TIMEOUT (10s) for exit().block() in close(); Objects.requireNonNullElse(builder.name, ...) in constructor.
  • McpAsyncClientWrapper: 10s timeout when closing the MCP client in close().
  • Toolkit: 30s timeout when registering MCP client in builder.

Testing: mvn spotless:apply and mvn test (or mvn verify) pass.

@no-0-name no-0-name requested a review from a team February 26, 2026 22:00
@cla-assistant
Copy link

cla-assistant bot commented Feb 26, 2026

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@gemini-code-assist
Copy link

Summary of Changes

Hello @no-0-name, 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 enhances the stability and consistency of the core module by introducing explicit timeouts for asynchronous blocking operations and standardizing null-checking practices. These changes mitigate potential deadlocks or indefinite waits in critical sections and align the codebase with modern Java utility usage, leading to a more predictable and maintainable system.

Highlights

  • Explicit Timeouts for Blocking Operations: Added specific Duration timeouts to all block() calls across core components, preventing indefinite blocking and improving application robustness. For instance, MsgHub close operations now have a 10-second timeout, knowledge retrieval has a 60-second timeout, and MCP client registration and closing operations have 30-second and 10-second timeouts respectively.
  • Consistent Null Checks with Objects Utilities: Refactored null checks to consistently use Objects.requireNonNull and Objects.requireNonNullElse for improved code readability and adherence to project style. This includes checks for Knowledge instances in KnowledgeRetrievalTools and default name assignment in MsgHub.
  • New Constants for Configuration: Introduced new constants like DEFAULT_RETRIEVE_LIMIT and RETRIEVE_TIMEOUT in KnowledgeRetrievalTools, and CLOSE_TIMEOUT in MsgHub to centralize and clarify configuration values.
Changelog
  • agentscope-core/src/main/java/io/agentscope/core/pipeline/MsgHub.java
    • Imported java.time.Duration and java.util.Objects.
    • Added a CLOSE_TIMEOUT constant for closing operations.
    • Replaced a conditional null check for builder.name with Objects.requireNonNullElse.
    • Applied CLOSE_TIMEOUT to the exit().block() call in the close() method and updated its Javadoc.
  • agentscope-core/src/main/java/io/agentscope/core/rag/KnowledgeRetrievalTools.java
    • Imported java.time.Duration and java.util.Objects.
    • Introduced DEFAULT_RETRIEVE_LIMIT and RETRIEVE_TIMEOUT constants.
    • Replaced an if (null) check for the knowledge constructor parameter with Objects.requireNonNull and updated the exception type in Javadoc.
    • Used Objects.requireNonNullElse to determine the effective retrieval limit.
    • Applied RETRIEVE_TIMEOUT to the retrieve().block() call.
  • agentscope-core/src/main/java/io/agentscope/core/tool/Toolkit.java
    • Imported java.time.Duration.
    • Added a 30-second timeout to the block() call when registering an MCP client.
  • agentscope-core/src/main/java/io/agentscope/core/tool/mcp/McpAsyncClientWrapper.java
    • Imported java.time.Duration.
    • Added a 10-second timeout to the block() call during graceful MCP client closing.
Activity
  • No specific activity (comments, reviews, etc.) was provided in the context.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request is a good step towards making the application more robust by adding timeouts to blocking calls and improving code style with Objects utilities. The changes in MsgHub and KnowledgeRetrievalTools are well-implemented, including the use of constants for timeout values. However, in Toolkit and McpAsyncClientWrapper, new timeouts have been introduced with magic numbers. I've left a couple of suggestions to extract these into named constants for better maintainability and consistency across the project.

groupName,
presetParameters)
.block();
.block(Duration.ofSeconds(30));

Choose a reason for hiding this comment

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

medium

To improve maintainability and for consistency with other changes in this PR, please extract the magic number 30 into a named constant within the ToolRegistration class. For example: private static final Duration MCP_REGISTRATION_TIMEOUT = Duration.ofSeconds(30);

Suggested change
.block(Duration.ofSeconds(30));
.block(MCP_REGISTRATION_TIMEOUT);

.doOnSuccess(v -> logger.debug("MCP client '{}' closed", name))
.doOnError(e -> logger.error("Error closing MCP client '{}'", name, e))
.block();
.block(Duration.ofSeconds(10));

Choose a reason for hiding this comment

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

medium

To improve maintainability and for consistency with other changes in this PR, please extract the magic number 10 into a named constant at the top of the McpAsyncClientWrapper class. For example: private static final Duration CLOSE_GRACEFULLY_TIMEOUT = Duration.ofSeconds(10);

Suggested change
.block(Duration.ofSeconds(10));
.block(CLOSE_GRACEFULLY_TIMEOUT);

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.

1 participant