improve abi parsing #213
improve abi parsing #213
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant GetABIForContract
participant HTTPServer
Caller->>GetABIForContract: Call with (chainId, contract)
GetABIForContract->>HTTPServer: Request ABI JSON
HTTPServer-->>GetABIForContract: Respond with JSON body
GetABIForContract->>GetABIForContract: Read full response body
GetABIForContract->>GetABIForContract: Lock mutex for JSON decode
GetABIForContract->>GetABIForContract: Decode ABI JSON
GetABIForContract->>GetABIForContract: Unlock mutex
GetABIForContract-->>Caller: Return parsed ABI or error
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 0
🧹 Nitpick comments (2)
internal/common/abi.go (2)
21-21: Consider the performance implications of the global mutex.While the mutex addresses the thread safety issue with
abi.JSON, using a global mutex will serialize ALL JSON decoding operations across the entire application, potentially creating a significant bottleneck under high concurrency.Consider these alternatives:
- Use a more granular locking mechanism (e.g., per-contract or per-chain mutex pool)
- Investigate if the
abi.JSONfunction can be made thread-safe internally- Use a worker pool pattern to limit concurrent ABI parsing operations
- jsonDecodeMutex sync.Mutex + // Consider using a sync.Map or mutex pool for more granular locking + abiParseMutexes sync.Map // map[string]*sync.Mutex for per-contract locking
83-88: Optimize string conversion and enhance error context.The current implementation has a minor performance issue with unnecessary string conversion and could benefit from more detailed error context.
Apply this optimization to avoid string conversion overhead:
- // Parse the ABI - parsedABI, err := abi.JSON(strings.NewReader(string(body))) + // Parse the ABI + parsedABI, err := abi.JSON(bytes.NewReader(body))Also add the
bytesimport:+ "bytes" "fmt"The enhanced error logging with contract and chain ID context is excellent for debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/common/abi.go(3 hunks)
🔇 Additional comments (3)
internal/common/abi.go (3)
5-5: LGTM: Required imports added correctly.The new imports for
ioandlogare necessary for the enhanced functionality.Also applies to: 15-15
72-76: Good approach to read the entire response body.Reading the entire response body before parsing improves stability and aligns with the goal of avoiding concurrent map write issues during streaming JSON decoding.
78-80: Mutex protection addresses the core thread safety issue.The mutex successfully serializes access to the
abi.JSONfunction, preventing the concurrent map write panics mentioned in the PR objectives.
# Simplify ABI Parsing Logic Refactors the contract ABI fetching logic in `GetABIForContract` by: - Removing the unnecessary step of reading the entire response body into memory - Parsing the ABI directly from the response body stream - Eliminating the JSON decode mutex that's no longer needed - Removing unused imports (`io` and `log`) This change makes the code more efficient by avoiding an extra memory allocation and simplifying the error handling path. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Improved the process for retrieving and parsing contract ABIs, resulting in a more efficient and streamlined experience when interacting with contract data. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
TL;DR
Added thread safety to ABI parsing to prevent concurrent map write panics.
What changed?
GetABIForContractfunctionHow to test?
Why make this change?
The
abi.JSONfunction internally uses maps that are not thread-safe. When multiple goroutines call this function concurrently, it can lead to "concurrent map writes" panics. This change adds a mutex to synchronize access to the JSON decoding process, preventing these race conditions while maintaining the ability to process requests concurrently.Summary by CodeRabbit