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

address client wallet hanging issues #1732

Merged
merged 3 commits into from Aug 4, 2022
Merged

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Jul 25, 2022

We're clearly fighting some limitations of our asset.Wallet interface (no ctx args) and Go (no context.Merge), but there are some smaller things we can do.

The first commit adds some more context passing to avoid things getting hung and unable to cancel them. Some sensible timeouts are also established.

The second commit is a bit of a hack to prevent shutdown from becoming impossible if an RPC or something hangs and causes deadlock on the trackedTrade mutex.

This PR is the basis for more extensive refactoring in #1739

@chappjc
Copy link
Member Author

chappjc commented Jul 27, 2022

Ultimately we probably should move toward adding ctx args on the interfaces like client/asset.Wallet and client/asset/btc.Wallet. Things would be a whole lot easier if there were a context.Merge, but that doesn't exists so I guess we should give up on trying to use an asset's own subsystem context and just accept the context from the caller i.e. Core.

Another challenge is how we do a TON with the trackedTrade mutex locked in (*Core).tick. Any wallet request hang is deadly because of that, not just because of logout, but many other handlers. That'll take a bit more careful refactoring, which we can do after branching release-v0.5. EDIT: A resolution to these design challenges is now in #1739

@chappjc chappjc linked an issue Jul 27, 2022 that may be closed by this pull request
@chappjc chappjc marked this pull request as ready for review July 28, 2022 23:28
client/core/trade.go Show resolved Hide resolved
client/core/trade.go Show resolved Hide resolved
@chappjc chappjc added this to the 0.5 milestone Jul 31, 2022
@chappjc chappjc merged commit 66f5f9c into decred:master Aug 4, 2022
@chappjc chappjc deleted the wallet-hang branch August 4, 2022 14:06
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.

client: Sometimes ignores shutdown signal.
3 participants