-
Notifications
You must be signed in to change notification settings - Fork 1
[TASK] Remove handler duplication between A2AServerImpl and DefaultA2AProtocolHandler #100
Copy link
Copy link
Closed
Copy link
Description
Problem Statement
There is unnecessary duplication and dead code in task handler management between A2AServerImpl and DefaultA2AProtocolHandler, creating maintenance burden and synchronization issues.
Current State Analysis
Architecture Issues
- Dead Code:
backgroundTaskHandlerfield inDefaultA2AProtocolHandler(line 348) is never used - Duplication:
streamingTaskHandlerstored in both server and protocol handler - Inconsistent Usage Pattern:
- Background tasks: Server uses
s.backgroundTaskHandlerdirectly (server.go:596) - Streaming tasks: Protocol handler uses its copy
h.streamingTaskHandler(task_handler.go:610)
- Background tasks: Server uses
- Synchronization Bug:
SetStreamingTaskHandler()only updates server copy, leaving protocol handler with stale reference
Code Evidence
// DefaultA2AProtocolHandler - lines 348-349
type DefaultA2AProtocolHandler struct {
backgroundTaskHandler TaskHandler // DEAD CODE - never referenced
streamingTaskHandler StreamableTaskHandler // Only used in HandleMessageStream
}
// A2AServerImpl stores the same handlers
type A2AServerImpl struct {
backgroundTaskHandler TaskHandler // Used in ProcessTask (line 596)
streamingTaskHandler StreamableTaskHandler // Never used directly by server
}Proposed Solutions
Option 1: Method Parameter Injection (Recommended)
Approach: Protocol handler methods receive handlers as parameters from server
- ✅ Eliminates duplication completely
- ✅ Single source of truth (server only)
- ✅ No synchronization issues
- ✅ Clean dependency injection
- ✅ Improved testability
// Protocol handler interface change
type A2AProtocolHandler interface {
HandleMessageStream(c *gin.Context, req types.JSONRPCRequest, handler StreamableTaskHandler)
// background tasks already use server's handler via task manager
}
// Server calls protocol handler
s.protocolHandler.HandleMessageStream(c, req, s.streamingTaskHandler)Impact Assessment
Benefits
- 🗑️ Eliminate Dead Code: Remove unused
backgroundTaskHandlerfrom protocol handler - 🎯 Single Source of Truth: All handlers managed by server only
- 🔄 Fix Synchronization: No more stale handler references
- 🏗️ Cleaner Architecture: Reduce coupling and improve maintainability
- ✅ Better Testability: Easier to mock and test handler interactions
Risk Analysis
- Low Risk: Only affects internal server architecture
- No Breaking Changes: Public API remains unchanged
- Improved Maintainability: Cleaner code structure
Acceptance Criteria
- Remove unused
backgroundTaskHandlerfield fromDefaultA2AProtocolHandler - Eliminate
streamingTaskHandlerduplication between server and protocol handler - Protocol handler gets streaming handler dynamically (not stored copy)
- Single source of truth for all task handlers (server only)
- No synchronization issues when
SetStreamingTaskHandler()is called - All existing tests continue to pass
- No breaking changes to public API
- Code follows clean architecture principles (dependency injection over service locator)
Implementation Notes
Files to Modify
server/task_handler.go(lines 342-369) - Protocol handler structure and constructorserver/server.go(lines 254-275) - Server handler managementserver/server_builder.go(lines 304-310) - Builder pattern updates- Test files using protocol handler
Testing Strategy
- Update existing protocol handler tests to use dependency injection
- Verify handler synchronization works correctly
- Test both background and streaming task processing flows
- Ensure no regression in task processing functionality
Definition of Done
- All linting checks pass (
task lint) - All tests pass (
task test) - Code review approved
- Documentation updated if needed
Priority: Medium
Complexity: Low
Category: Technical Debt / Refactoring
Reactions are currently unavailable
Metadata
Metadata
Assignees
Type
Projects
Status
Done