Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Eradicate zombie RPC threads#31688

Merged
mergify[bot] merged 3 commits intosolana-labs:masterfrom
CriesofCarrots:fix-zombie-threads
May 17, 2023
Merged

Eradicate zombie RPC threads#31688
mergify[bot] merged 3 commits intosolana-labs:masterfrom
CriesofCarrots:fix-zombie-threads

Conversation

@CriesofCarrots
Copy link
Copy Markdown
Contributor

Problem

Cluster simulations still exhibit zombie threads in the rpc_service/send_transaction_service area when the recently removed AccountsHashVerifier::should_halt() is triggered. This is because this change is only exercised when a formal ValidatorExit is triggered, and not when the exit AtomicBool is set to true.

Summary of Changes

  • Call JsonRpcService::exit() inside JsonRpcService::join(), to ensure the Server actually closes when cleaning up threads
  • Also clean up a slight regression introduced in Change JsonRpc exit to use wait->close #5566 and perpetuated in Register SendTransactionService exit #31261 wherein the SendTransactionService uses an rpc-service-specific exit, instead of the same exit as all the other services. (This did not appear to be causing zombie threads, however.)

Copy link
Copy Markdown
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotta be at least a step in the right direction 😉

@CriesofCarrots
Copy link
Copy Markdown
Contributor Author

gotta be at least a step in the right direction 😉

@jeffwashington pointed me at a way to repro consistently, so I have a little more confidence this time 😅

@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label May 17, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented May 17, 2023

Codecov Report

Merging #31688 (fc17d5a) into master (b422ac0) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #31688   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         736      736           
  Lines      205962   205963    +1     
=======================================
+ Hits       168661   168665    +4     
+ Misses      37301    37298    -3     

@mergify mergify Bot merged commit 2cdb43f into solana-labs:master May 17, 2023
mergify Bot pushed a commit that referenced this pull request May 17, 2023
* Ensure jsonrpc server has closed when joining rpc_service thread

* Use same exit bool as other services

* Remove redundant registered exit line

(cherry picked from commit 2cdb43f)

# Conflicts:
#	core/src/validator.rs
CriesofCarrots pushed a commit that referenced this pull request May 18, 2023
* Eradicate zombie RPC threads (#31688)

* Ensure jsonrpc server has closed when joining rpc_service thread

* Use same exit bool as other services

* Remove redundant registered exit line

(cherry picked from commit 2cdb43f)

# Conflicts:
#	core/src/validator.rs

* Fix conflict

* Remove redundant clone

---------

Co-authored-by: Tyera <tyera@solana.com>
wen-coding pushed a commit to wen-coding/solana that referenced this pull request May 18, 2023
* Ensure jsonrpc server has closed when joining rpc_service thread

* Use same exit bool as other services

* Remove redundant registered exit line
@CriesofCarrots CriesofCarrots deleted the fix-zombie-threads branch May 25, 2023 22:11
bw-solana pushed a commit to bw-solana/solana that referenced this pull request Jan 10, 2025
…olana-labs#31689)

* Eradicate zombie RPC threads (solana-labs#31688)

* Ensure jsonrpc server has closed when joining rpc_service thread

* Use same exit bool as other services

* Remove redundant registered exit line

(cherry picked from commit 2cdb43f)

# Conflicts:
#	core/src/validator.rs

* Fix conflict

* Remove redundant clone

---------

Co-authored-by: Tyera <tyera@solana.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

automerge Merge this Pull Request automatically once CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants