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

refactor(client): remove manual client arc counting #4484

Merged
merged 2 commits into from
Mar 8, 2024

Conversation

dpc
Copy link
Contributor

@dpc dpc commented Mar 6, 2024

On top of #4482

Use two levels of Arc instead. While at it, rename and document things
better.

@dpc dpc requested review from a team as code owners March 6, 2024 21:34
@dpc dpc force-pushed the 24-03-06-fix-client-shutdown-2 branch 3 times, most recently from 85223bf to aa30514 Compare March 7, 2024 03:22
elsirion
elsirion previously approved these changes Mar 7, 2024
Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

I think in total the PR is good, just one intermediately broken commit.

fedimint-client/src/lib.rs Outdated Show resolved Hide resolved
fedimint-client/src/lib.rs Show resolved Hide resolved
Fundamentally we can't rely on database count to detect when
client executors (state machines and recoveries) finished,
because external user might hold some references too.

Also, we must not be dropping `inner : Arc<Client>` before
we ensure executors stopped, because something running in them
might still require that `Arc<Client>` is alive and weak references
to them can be upgraded.

Instead use the old and (somewhat) trusted TaskGroup to do the
job.
Use two levels of `Arc` instead. While at it, rename and document things
better.
@dpc dpc force-pushed the 24-03-06-fix-client-shutdown-2 branch from d068ad5 to 3f4f585 Compare March 7, 2024 07:12
@dpc
Copy link
Contributor Author

dpc commented Mar 7, 2024

I think in total the PR is good, just one intermediately broken commit.

Fixed at first, but then decided to squash the 3 last commits, as it doesn't help anything to have a commit with a problem.

@dpc dpc requested a review from elsirion March 7, 2024 07:13
Copy link
Member

@Kodylow Kodylow left a comment

Choose a reason for hiding this comment

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

tACK

@dpc dpc added this pull request to the merge queue Mar 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 8, 2024
@dpc dpc added this pull request to the merge queue Mar 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 8, 2024
@dpc dpc added this pull request to the merge queue Mar 8, 2024
Merged via the queue into fedimint:master with commit 0ab2634 Mar 8, 2024
20 checks passed
@dpc dpc deleted the 24-03-06-fix-client-shutdown-2 branch March 8, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants