Make handleEvent an async method #701
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
This is the first in a series of steps toward making [PerAccountStore.handleEvent] an async method. We'll need that in turn in order to let it start saving changes to disk when applicable, like for zulip#458, and having both it and its caller, [UpdateMachine.poll], wait for the result rather than barreling ahead to make other changes concurrently. This change comes first because when we make `handleEvent` async, we'll want to make sure all its call sites properly wait for it to finish; and when that requires changing the caller to an async method, we'll want to do the same for *its* call sites, recursively. To keep the changes manageable to read and review, we'll make separate commits for each method, except local helpers with only a handful of call sites. The callers have to go before the callees in order to keep the commits coherent, so we start with this transitive caller.
Generally the application code should be run inside test cases, and not from the test-discovery code that runs before the test harness even knows what the list of test cases is. That means it should be called from inside a `test` callback, not from `main` itself or from a `group` callback. There's a lot of application code that runs as part of calls like `store.addStreams` and `store.addUser`. So this `setupStore` method should be called from inside the test cases. Do that. In particular this is needed in order to make an async method out of PerAccountStore.handleEvent, which underlies those helpers like `addStreams`. Async calls especially need to be kept out of test-discovery time, because `package:test` requires all the test discovery to be completed synchronously.
This is the next step toward making handleEvent async.
This is the next step toward making handleEvent async.
This is the next step toward making handleEvent async.
This is the next step toward making handleEvent async.
This is the next step toward making handleEvent async.
This is the next step toward making handleEvent async.
This is the next step toward making handleEvent async.
With zulip#458, we'll want this method to write changes to the database on certain events, to update the metadata we keep about the user's Zulip servers. When it does that, we'll want it to wait for that write to finish before it moves on to make other changes, and for its caller [UpdateMachine.poll] to similarly wait. That saves us from having to worry about potential races if some later event also wants to make database changes while the first changes haven't finished yet. It also gives us a good foundation for generally not showing the user a confirmation that some change has been made until we've actually saved the change. So to do that, we'll need the method to be async, and to return a Future which its call sites should wait for. That's this commit. It completes a series where we did the same thing for its callers, recursively.
|
Interesting; yeah, I guess the need for this makes sense. LGTM, 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.
With #458, we'll want this method to write changes to the database
on certain events, to update the metadata we keep about the user's
Zulip servers.
When it does that, we'll want it to wait for that write to finish
before it moves on to make other changes, and for its caller
[UpdateMachine.poll] to similarly wait. That saves us from having
to worry about potential races if some later event also wants to
make database changes while the first changes haven't finished yet.
It also gives us a good foundation for generally not showing the
user a confirmation that some change has been made until we've
actually saved the change.
So to do that, we'll need the method to be async, and to return a
Future which its call sites should wait for.
This PR does that, after a series of commits donig the same for its
callers, recursively.