Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assess the need for replacing async_service #525

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

paschal533
Copy link
Contributor

@paschal533 paschal533 commented Feb 26, 2025

Assess the need for replacing async_service

This PR addresses the issue of evaluating alternatives to the current async_service implementation in this project, which was copied from the now unmaintained ethereum/async-service library.

Analysis

After reviewing the codebase, I found that async_service provides critical functionality for managing long-running services in this project, including:

  • Service lifecycle management
  • Structured task management with cancellation support
  • Child service relationships
  • Background task execution with error handling
  • Stats tracking

Recommendation

I recommend replacing the current implementation with a simpler, more maintainable version based on anyio. The benefits include:

  1. Reduced complexity: The new implementation is significantly simpler (~400 lines vs. ~1000 lines)
  2. Backend agnostic: Works with both Trio and asyncio through anyio's abstraction
  3. Modern structured concurrency: Uses anyio's task groups for cleaner task management
  4. Maintained dependency: anyio is actively maintained and well-documented
  5. Preserved API: Existing code using async_service would require minimal changes

Proof of Concept

This PR includes a proof-of-concept implementation in the proof-of-concept/ directory. It demonstrates how the core functionality can be preserved while simplifying the implementation.

The PoC shows:

  • How the API surface is preserved
  • The use of anyio's structured concurrency primitives
  • Simplified task tracking and cancellation
  • Example usage patterns

Questions for Maintainers

  1. Is introducing anyio as a dependency acceptable?
  2. Are there any specific features of the current implementation that must be preserved?
  3. What would be the preferred migration strategy if this approach is accepted?

Next Steps

If this approach is accepted, I would implement a comprehensive test suite and provide a full implementation as a follow-up PR.

issue #524

@paschal533 paschal533 marked this pull request as draft March 3, 2025 10:46
@paschal533 paschal533 marked this pull request as ready for review March 3, 2025 10:47
@pacrob
Copy link
Member

pacrob commented Mar 16, 2025

This looks good @paschal533. anyio is an acceptable dependency to add. And it appears that by replicating the API from async_service, the change should be pretty much seamless. As long as that is true, I'd say go ahead. @seetadev, any thoughts or concerns here?

@seetadev
Copy link

This looks good @paschal533. anyio is an acceptable dependency to add. And it appears that by replicating the API from async_service, the change should be pretty much seamless. As long as that is true, I'd say go ahead. @seetadev, any thoughts or concerns here?

@pacrob : The proposal shared by @paschal533 is well-structured and thoughtfully addresses the problem, but there are some important considerations and potential concerns to address:

Strengths

  1. Thorough analysis – The feature enhancement by @paschal533 correctly identifies that async_service provides essential functionality (like structured concurrency, task cancellation, and error handling).
  2. Clear recommendation – Recommending anyio is a solid choice since it’s modern, well-maintained, and supports both asyncio and trio backends.
  3. Compatibility Preservation – Preserving the existing API minimizes friction for downstream projects and avoids a costly migration.
  4. Simplification – Reducing the code size from ~1000 to ~400 lines should improve maintainability and reduce the surface area for bugs.

⚠️ Concerns and Open Questions

  1. Dependency Impact

    • Adding anyio introduces a new dependency. While anyio is well-maintained, it adds a potential layer of abstraction over asyncio and trio.
    • Compatibility: Are there any known compatibility issues between anyio and other libraries already used in py-libp2p? For example, anyio might introduce subtle conflicts if other asyncio-based libraries are tightly integrated.
  2. Performance Considerations

    • anyio’s abstraction layer might introduce some minor overhead compared to a direct asyncio implementation. Has the PoC shown any measurable performance impact (positive or negative)?
    • How does task scheduling and cancellation performance compare with async_service under load?
  3. Backward Compatibility

    • Preserving the API surface is good — but some internal behaviors (like error handling and task lifecycle) might differ subtly.
    • Have you confirmed that existing test cases using async_service pass without changes when using the anyio-based implementation?
  4. Maintenance and Ecosystem Fit

    • Is the anyio abstraction stable enough for long-term support?
    • If anyio changes behavior or introduces breaking changes, how easy will it be to update the integration?
  5. Testing and Migration Strategy

    • How comprehensive is the test coverage for service lifecycle, error propagation, and cancellation?
    • A phased rollout or feature flag might be useful.

@paschal533 : Wish if you could do the following tasks and share here:

  1. ✅ Run performance benchmarks (e.g., task scheduling, cancellation, and load tests) to confirm that anyio performs similarly or better than async_service.
  2. ✅ Create a compatibility matrix to verify that anyio works well alongside other asyncio-based dependencies in py-libp2p.
  3. ✅ Expand the feature enhancement into a full test suite to validate lifecycle, concurrency, and error-handling scenarios.

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.

3 participants