Register SendTransactionService exit #31261
Conversation
CriesofCarrots
force-pushed
the
sts-exit
branch
2 times, most recently
from
47ef9e9 to
9a6af2f
Compare
Codecov Report
@@ Coverage Diff @@
## master #31261 +/- ##
=======================================
Coverage 81.5% 81.5%
=======================================
Files 733 733
Lines 206885 206926 +41
=======================================
+ Hits 168693 168769 +76
+ Misses 38192 38157 -35 |
|
Follow-up work will examine exit and join behavior in JsonRpcService |
| ); | ||
|
|
||
| exit.store(true, Ordering::Relaxed); |
There was a problem hiding this comment.
i think exit is going to race the thread spawn here, should probably wait on... something.
similarly need to sit on join() after setting exit to ensure the thread stops as intended.
There was a problem hiding this comment.
should probably wait on... something.
i guess easiest would be to switch to a zero-capacity bounded channel and do a blocking write to it. once thread is up, that message will be consumed and waiter will unblock. at that point it's safe to signal exit
There was a problem hiding this comment.
Yep, great idea. Done.
CriesofCarrots
force-pushed
the
sts-exit
branch
from
9a6af2f to
8e53077
Compare
CriesofCarrots
force-pushed
the
sts-exit
branch
2 times, most recently
from
94263fa to
e3d25a3
Compare
CriesofCarrots
force-pushed
the
sts-exit
branch
from
e3d25a3 to
c825b6b
Compare
| let mut option = Ok(()); | ||
| while option.is_ok() { | ||
| option = sender.send(dummy_tx_info()); | ||
| } |
There was a problem hiding this comment.
i'm guessing we do this 'cause SendTransactionService::join() looks busted and that will be addressed in the alluded to followup?
There was a problem hiding this comment.
Naw, join() looks fine, actually. Ensuring the receiver was dropped was the best thing I thought of to show that updating exit outside of the service actually causes stuff to stop. join() already sets exit directly. Open to better ideas, though!
There was a problem hiding this comment.
...post-merge ;)
There was a problem hiding this comment.
Turns out this was wrong. join() also needs to ensure the rpc Server is closed.
* Pass exit into SendTransactionService * Abort SendTransactionService with BanksService * Register SendTransactionService exit as part of RpcService validator Exit * Improve test, ensure receiver has been dropped (cherry picked from commit 03c1744) # Conflicts: # rpc/src/rpc_service.rs
) * Register SendTransactionService exit (#31261) * Pass exit into SendTransactionService * Abort SendTransactionService with BanksService * Register SendTransactionService exit as part of RpcService validator Exit * Improve test, ensure receiver has been dropped (cherry picked from commit 03c1744) # Conflicts: # rpc/src/rpc_service.rs * Fix conflict --------- Co-authored-by: Tyera <tyera@solana.com>
…31261) (solana-labs#31292) * Register SendTransactionService exit (solana-labs#31261) * Pass exit into SendTransactionService * Abort SendTransactionService with BanksService * Register SendTransactionService exit as part of RpcService validator Exit * Improve test, ensure receiver has been dropped (cherry picked from commit 03c1744) # Conflicts: # rpc/src/rpc_service.rs * Fix conflict --------- Co-authored-by: Tyera <tyera@solana.com>
Problem
The SendTransactionService doesn't receive exit signals from the validator, allowing the threads to continue in zombie mode when the validator has otherwise halted.
Summary of Changes
Extend the JsonRpcService exit block to signal SendTransactionService when the validator exits, by passing in an AtomicBool instead of creating on inside SendTransactionService
Fixes #31079