Blockstore should drop signals before validator exit

[HaoranYi]

Problem

blockstore holds two set of signal senders, and blockstore is Arc in many places, including RpcSubscription. During shutting down, a deadlock may occurs, when the signal receiver is using a blocking call and the blockstore ref count never goes to 0.

Summary of Changes

• Add drop_signals function on blockstore. When exiting, drop the signals to avoid deadlock.
• Change signals to be protected by mutex.

Fixes #

[jeffwashington]

I think you can say Mutex::default() here.

[jeffwashington]

is this solving test hanging? I don't recall the context.
Does RwLock work better here or is Mutex appropriate? I'm not familiar with the usage patterns in the normal validator.

[codecov]

Codecov Report

Merging #24025 (a3e8a33) into master (51b37f0) will increase coverage by 0.0%.
The diff coverage is 100.0%.

❗ Current head a3e8a33 differs from pull request most recent head c6ef522. Consider uploading reports for the commit c6ef522 to get more accurate results

@@           Coverage Diff           @@
##           master   #24025   +/-   ##
=======================================
  Coverage    81.7%    81.7%           
=======================================
  Files         589      589           
  Lines      160603   160622   +19     
=======================================
+ Hits       131236   131256   +20     
+ Misses      29367    29366    -1     

[HaoranYi]

is this solving test hanging? I don't recall the context. Does RwLock work better here or is Mutex appropriate? I'm not familiar with the usage patterns in the normal validator.

Yes. This one is a follow up of the #24007.

In #24007, we fix the hang by using nonblocking call inside RpcCompletedSlotService.

This change implement the idea suggested by @sakridge - when shutting down, we drop all the signal senders in blockstore, so that all receivers whether blocking or nonblocking will return on receive.

In this case, I don't think RwLock and Mutex will make much difference. The vec inside the lock is only written twice, once at the initialization, and once cleared at the shutdown. And there is only one thread reading it inside blockstore.

Let me add @carllin see if there is any other similar patterns in the validator.

[carllin]

Hmm, is this going to fix anything? I feel like the validator exit bool should be sufficient for triggering shutdown.

The blockstore signals are created here:

solana/core/src/validator.rs

Lines 495 to 496 in 51b37f0

	ledger_signal_receiver,
	completed_slots_receiver,

The consumers are:

1. RPC

   solana/core/src/validator.rs

   Lines 875 to 879 in 51b37f0

   	let rpc_completed_slots_service = RpcCompletedSlotsService::spawn(
   	completed_slots_receiver,
   	rpc_subscriptions.clone(),
   	exit.clone(),
   	);

   which also gets an exit signal
2. The tvu:

   solana/core/src/validator.rs

   Line 895 in 51b37f0

   ledger_signal_receiver,

   which also gets an exit signal

Thus it seems like setting the exit flag from the validator exit:

solana/core/src/validator.rs

Line 1006 in c6ef522

self.validator_exit.write().unwrap().exit();

should be sufficient

[HaoranYi]

@carllin
You are right. This one doesn't fix anything since #24007 already have fixed the hang. The rpc code you listed above is recent change #24007, that is used to fix the hang on the blocking call.

This one can be thought of as more of refactor and protection.

• We make the signal vec the blockstore private and expose API for access them
• We clear the signal vec before shutdown, which protect potential future changes of blocking on receiver, and it might help to speedup the shutdown (no need to wait till timeout)

Also, while refactoring this, I notice another potential issue #24051
We could potentially missing waking up signals in replaying stage. Please help to take a look at this as well. Let me know what do you think. Thanks!

[carllin]

@HaoranYi understood, thanks for explaining!