Don't construct or notify RpcSubscriptions when the RPC is off#6516
Conversation
| let rpc_subscriptions = Arc::new(RpcSubscriptions::new_with_config( | ||
| exit.clone(), | ||
| max_complete_transaction_status_slot.clone(), | ||
| blockstore.clone(), | ||
| bank_forks.clone(), | ||
| block_commitment_cache.clone(), | ||
| optimistically_confirmed_bank.clone(), | ||
| &config.pubsub_config, | ||
| None, | ||
| )); | ||
|
|
| let rpc_subscriptions = Arc::new(RpcSubscriptions::new_with_config( | ||
| exit.clone(), | ||
| max_complete_transaction_status_slot, | ||
| blockstore.clone(), | ||
| bank_forks.clone(), | ||
| block_commitment_cache.clone(), | ||
| optimistically_confirmed_bank.clone(), | ||
| &config.pubsub_config, | ||
| None, | ||
| )); |
steveluscher
force-pushed
the
rpc-off-rpc-subscriptions-go-poof
branch
from
20aac86 to
6f39b24
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6516 +/- ##
=========================================
- Coverage 82.8% 82.8% -0.1%
=========================================
Files 851 851
Lines 379955 379981 +26
=========================================
+ Hits 314717 314732 +15
- Misses 65238 65249 +11 🚀 New features to boost your workflow:
|
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
The bulk looks good! Mostly some style points and better ways to work with Option<Arc<T>>
| let mut latest_vote_slot_per_validator = HashMap::new(); | ||
| let mut last_process_root = Instant::now(); | ||
| let duplicate_confirmed_slot_sender = Some(duplicate_confirmed_slot_sender); | ||
| let subscriptions = subscriptions.and_then(|arc| Arc::try_unwrap(arc).ok()); |
There was a problem hiding this comment.
If you want to convert from Option<Arc<T>> to Option<&T>, it's better to use as_deref() https://doc.rust-lang.org/std/option/enum.Option.html#method.as_deref
This means changing the &Option<T> to Option<&T> and using as_deref() in all call sites, or you can use &Option<&T>, but I don't think there's any benefit to that.
try_unwrap() will fail if there's other references to the RpcSubscriptions, so we should avoid it: https://doc.rust-lang.org/std/sync/struct.Arc.html#method.try_unwrap
| receiver: &Receiver<CommitmentAggregationData>, | ||
| block_commitment_cache: &RwLock<BlockCommitmentCache>, | ||
| subscriptions: &Arc<RpcSubscriptions>, | ||
| rpc_subscriptions: &Option<Arc<RpcSubscriptions>>, |
There was a problem hiding this comment.
This isn't your fault, but &Arc<T> is an antipattern, and typically should just use &T instead. How about changing this to Option<&RpcSubscriptions>?
| poh_recorder: &Arc<RwLock<PohRecorder>>, | ||
| leader_schedule_cache: &Arc<LeaderScheduleCache>, | ||
| rpc_subscriptions: &Arc<RpcSubscriptions>, | ||
| rpc_subscriptions: &Option<Arc<RpcSubscriptions>>, |
There was a problem hiding this comment.
On the flipside, probably not worth changing here for consistency with every other arg 🙃
| &bank_forks, | ||
| &leader_schedule_cache, | ||
| &rpc_subscriptions, | ||
| &Some(rpc_subscriptions.clone()), |
There was a problem hiding this comment.
nit: These test changes would probably be simpler by doing let rpc_subscriptions = Some(rpc_subscriptions); earlier on, and then passing &rpc_subscriptions as before. Like at line 5193
| &vote_tracker, | ||
| &bank3, | ||
| &subscriptions, | ||
| &Arc::try_unwrap(subscriptions.clone()).ok(), |
There was a problem hiding this comment.
These tests will get much simpler too, since you can just pass subscriptions.as_deref()
|
Sorry for leading you astray, the error message in your branch didn't tell the real problem, which was that we were trying to move a reference into a new thread, which is not possible because refs aren't safe to move into new threads. Some nice info at https://users.rust-lang.org/t/question-on-moving-a-reference-to-a-child-thread/47387 I changed some of the callsites back to Let me know what you think! |
|
Thank you! Stamp and I'll ship. |
|
Ah right, we'll need someone else to approve because I was the last person to push to the branch |
…a-xyz#6516) * Don't construct or notify `RpcSubscriptions` when the RPC is off * Less dumb Rust * Keep going? Can't get it to typecheck. * Revert some places that still need `Arc`s due to threads * Cleanup tests, avoid a clone in tpu --------- Co-authored-by: Jon C <me@jonc.dev>
4 participants
Problem
It appears as though we unconditionally construct
RpcSubscriptions, and push notifications into it, despite the node not having RPC enabled.Summary of Changes
Turn it into an
Option. Only construct it in the block where we set up the rest of the RPC. Don't notify if thatOptionisNone.