Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v1.18: Fix BankForks::new_rw_arc memory leak (backport of #1893) #2065

Closed
wants to merge 1 commit into from

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented Jul 9, 2024

Problem

BankForks::new_rw_arc leaks memory because BankForks holds an Arc to a Bank and the Bank holds an Arc to BankForks through the Bank::transaction_processor: TransactionBatchProcessor<BankForks>, TransactionProcessor<FG: ForkGraph>::program_cache: Arc<RwLock<ProgramCache<FG>>>, ProgramCache<FG: ForkGraph>::fork_graph: Option<Arc<RwLock<FG>>> reference cycle.

Summary of Changes

Changed the fork_graph field to pub fork_graph: Option<Weak<RwLock<FG>>> to break the reference cycle. The test_bank_forks_new_rw_arc_memory_leak fails before the bug fix because of leaked threads from thread pools inside AccountsDb.

This bug has been introduced in solana-labs#33776. This bug affects also v1.18 and v1.17.


This is an automatic backport of pull request #1893 done by Mergify.

(cherry picked from commit d441c0f)

# Conflicts:
#	core/benches/banking_stage.rs
#	core/benches/consumer.rs
#	core/src/banking_stage/consume_worker.rs
#	core/src/banking_stage/consumer.rs
#	core/tests/unified_scheduler.rs
#	ledger/benches/blockstore_processor.rs
#	ledger/src/blockstore_processor.rs
#	program-runtime/src/loaded_programs.rs
#	programs/sbf/tests/programs.rs
#	runtime/src/bank.rs
#	runtime/src/bank/tests.rs
#	runtime/src/bank_forks.rs
#	svm/src/transaction_processor.rs
#	svm/tests/conformance.rs
#	svm/tests/integration_test.rs
#	unified-scheduler-pool/src/lib.rs
@mergify mergify bot requested a review from a team as a code owner July 9, 2024 20:31
@mergify mergify bot added the conflicts label Jul 9, 2024
@mergify mergify bot assigned pgarg66 Jul 9, 2024
Copy link
Author

mergify bot commented Jul 9, 2024

Cherry-pick of d441c0f has failed:

On branch mergify/bp/v1.18/pr-1893
Your branch is up to date with 'origin/v1.18'.

You are currently cherry-picking commit d441c0f577.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   bench-tps/src/bench.rs
	modified:   core/src/banking_stage.rs
	modified:   core/src/banking_stage/decision_maker.rs
	modified:   core/src/banking_stage/read_write_account_set.rs
	modified:   core/src/banking_stage/unprocessed_transaction_storage.rs
	modified:   poh/src/poh_service.rs
	modified:   rpc/src/transaction_status_service.rs
	modified:   runtime/src/bank_client.rs
	modified:   runtime/tests/stake.rs
	modified:   stake-accounts/src/stake_accounts.rs

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	both modified:   core/benches/banking_stage.rs
	both modified:   core/benches/consumer.rs
	both modified:   core/src/banking_stage/consume_worker.rs
	both modified:   core/src/banking_stage/consumer.rs
	deleted by us:   core/tests/unified_scheduler.rs
	both modified:   ledger/benches/blockstore_processor.rs
	both modified:   ledger/src/blockstore_processor.rs
	both modified:   program-runtime/src/loaded_programs.rs
	both modified:   programs/sbf/tests/programs.rs
	both modified:   runtime/src/bank.rs
	both modified:   runtime/src/bank/tests.rs
	both modified:   runtime/src/bank_forks.rs
	deleted by us:   svm/src/transaction_processor.rs
	deleted by us:   svm/tests/conformance.rs
	deleted by us:   svm/tests/integration_test.rs
	both modified:   unified-scheduler-pool/src/lib.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@pgarg66 pgarg66 removed their assignment Jul 9, 2024
@pgarg66
Copy link

pgarg66 commented Jul 9, 2024

@andreisilviudragnea the cherry-pick to the v1.18 branch has merge conflicts. Would you like to triage it?
Backport to v2.0 was clean. If that's sufficient for your needs, we can abandon this PR.

@andreisilviudragnea
Copy link

@pgarg66 I will try to triage it over the weekend.

@t-nelson
Copy link

this is not a backport candidate to any branch

@pgarg66
Copy link

pgarg66 commented Aug 21, 2024

Closing this PR, as backports to v1.18 is limited to critical fixes.

@pgarg66 pgarg66 closed this Aug 21, 2024
@mergify mergify bot deleted the mergify/bp/v1.18/pr-1893 branch August 21, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants