store: Implement removeAccount #1007
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We'll add more to this soon.
chrisbobbe
added
the
integration review
gnprice
reviewed
Oct 18, 2024
lib/model/store.dart
Outdated
Comment on lines
916
to
920
| BackoffMachine? backoffMachine; | ||
|
|
||
| while (true) { | ||
| if (_disposed) return; | ||
|
|
||
| if (_debugLoopSignal != null) { |
There was a problem hiding this comment.
(followup on #948 (comment) )
Suggested change
| BackoffMachine? backoffMachine; | |
| while (true) { | |
| if (_disposed) return; | |
| if (_debugLoopSignal != null) { | |
| assert(!_disposed); | |
| BackoffMachine? backoffMachine; | |
| while (true) { | |
| if (_debugLoopSignal != null) { |
I think a check at the top of the loop shouldn't be needed — it's not in itself right after an await, and we already have checks after each await.
Conversely an assert at the top of the whole method is good for the same reason as on other methods: it helps us catch if we're attempting to call the method when the object is already disposed, which would be a bug.
There was a problem hiding this comment.
Oh indeed! Reading more closely, I see you did suggest putting it at the top of the method, not the top of the loop.
This seems hygienic. It makes sense to explicitly clean up an old HTTP client's resources when we've set up a new one, for the dead-queue reload. (The dead-queue reload is currently the only path where this dispose method is called.) It will also make sense to clean up these resources when an account is logged out. That's a feature we'll be implementing soon, with PerAccountStore.dispose in that new path as well. The "abort long-poll and close ApiConnection" TODO was on UpdateMachine's dispose method, not PerAccountStore's. But since PerAccountStore is what owns the connection, its dispose method seemed like the more appropriate place to close the connection.
chrisbobbe
force-pushed
the
pr-remove-account
branch
from
a3b2e7c to
03dfdf1
Compare
|
Thanks for the review! Revision pushed. |
|
Thanks! Looks good; merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR was split out of #948, following #948 (review) , toward getting things merged more efficiently.
Related: #948