Revive the previous Filecoin.EthSubscribe PR#5749
Conversation
…field (a.k.a. main)
WalkthroughSupport for Ethereum-compatible pubsub subscriptions was added, including new RPC methods Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC_Server
participant PubSub_Handler
participant ChainStore
Client->>RPC_Server: eth_subscribe("newHeads" or "logs", [filter])
RPC_Server->>PubSub_Handler: Parse subscription, accept
PubSub_Handler->>ChainStore: Subscribe to head/log events
ChainStore-->>PubSub_Handler: Broadcast events (new head/logs)
loop On event
PubSub_Handler->>RPC_Server: Send JSON-RPC notification
RPC_Server->>Client: Notify (new head/logs)
end
Client->>RPC_Server: eth_unsubscribe
RPC_Server->>PubSub_Handler: Terminate subscription
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
elmattic
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
src/rpc/methods/eth/pubsub.rs
Outdated
| Ok(v) => { | ||
| match jsonrpsee::SubscriptionMessage::new("eth_subscription", sink.subscription_id(), &v) { | ||
| Ok(msg) => { | ||
| // This fails only if the connection is closed |
There was a problem hiding this comment.
perhaps instead of a comment we can have a log message?
LesnyRumcajs
left a comment
LesnyRumcajs
left a comment
There was a problem hiding this comment.
LGTM, though I'd love if there could be some unit/component tests for this feature.
Yes, I'd like them to be part of the api run subcommand. I've created some issue here: #5795 |
![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
♻️ Duplicate comments (1)
src/rpc/methods/eth/pubsub.rs (1)
140-142: Consider adding error handling for task spawn failures.Same concern as the new heads handler - consider adding error handling for task spawn failures.
🧹 Nitpick comments (2)
src/rpc/methods/eth/pubsub.rs (2)
126-129: Consider adding error handling for task spawn failures.While task spawning rarely fails, adding error handling would make the code more robust for edge cases like resource exhaustion.
- tokio::spawn(async move { - handle_subscription(subscriber, accepted_sink, handle).await; - }); + let task_handle = tokio::spawn(async move { + handle_subscription(subscriber, accepted_sink, handle).await; + }); + if task_handle.is_finished() { + tracing::warn!("Failed to spawn new heads subscription task"); + }
174-175: Consider logging lagged message events.While ignoring lagged messages is often acceptable for real-time subscriptions, logging these events could provide valuable monitoring insights about system load and client performance.
Err(RecvError::Lagged(_)) => { + tracing::debug!("Subscription lagged, skipping messages (id: {:?})", sink.subscription_id()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/rpc/channel.rs(1 hunks)src/rpc/methods/eth/pubsub.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Check
- GitHub Check: All lint checks
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
src/rpc/channel.rs (1)
370-372: LGTM! Enhanced error logging for subscription failures.The change from a simple error check to explicit error logging provides better observability for subscription failures while maintaining the same control flow. This aligns well with the similar error handling pattern already used elsewhere in the function.
src/rpc/methods/eth/pubsub.rs (3)
1-71: Well-documented API with clear sequence diagram.The comprehensive documentation with sequence diagrams effectively explains the Ethereum pubsub protocol flow, making it easy for future maintainers to understand the implementation.
87-114: Proper error handling and subscription dispatch logic.The subscription method correctly handles the different subscription types:
- Uses
?operator for proper error propagation frompending.accept()- Properly rejects unimplemented
pendingTransactionswith a clear error message- Clean dispatch to specific handlers for implemented types
This addresses the previous review concerns about unwrap() usage and pendingTransactions handling.
146-186: Robust subscription lifecycle management.The generic subscription handler demonstrates good practices:
- Proper use of
tokio::select!for concurrent event handling- Graceful handling of different error conditions (closed channel, lagged messages)
- Task cleanup by aborting the producer handle when subscription ends
- Informative logging for subscription lifecycle events
The error handling covers all relevant scenarios and ensures resources are properly cleaned up.
akaladarshi
left a comment
akaladarshi
left a comment
There was a problem hiding this comment.
Just a single comment. LGTM
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
4 participants
Summary of changes
Changes introduced in this pull request:
Filecoin.EthSubscribeand verify against Lotus #4122headsandlogs.Note:
pendingTransactionsevent type: will be implemented in refactor: use macros to register the subscription APIs #5782.api runsubcommand and a WebSocket client currently being developed in Addforest api test-statefulsubcommand #5836.Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation