Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2642493003:
[Sync] Clean up SMTP startup flow. (Closed)
Description[Sync] Clean up SMTP startup flow.
- start_error_ renamed model_error_, is always set on error, never
cleared, and is the permanent indicator that the model is in an
error state.
- ConnectIfReady now no-ops if called more than once. This could
happen if the get pending data callback was called synchronously
and was the source of the crash in 679657.
- Regression test added for the above issue.
- std::move() the key lists for GetData calls for efficiency.
- Cleaned up DCHECKS a bit and used ASSERT or EXPECT in tests instead.
- Remove EntityMap and UpdateMap typedefs in the SMTP header.
BUG=673883, 679657
Review-Url: https://codereview.chromium.org/2642493003
Cr-Commit-Position: refs/heads/master@{#444561}
Committed: https://chromium.googlesource.com/chromium/src/+/d462fc419f40c24915a6c32cea50ecb3c5273f65
Patch Set 1 #
Total comments: 20
Patch Set 2 : Address comments. #
Total comments: 6
Patch Set 3 : Address comments. #
Dependent Patchsets: Messages
Total messages: 21 (14 generated)
The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
maxbogue@chromium.org changed reviewers: + skym@chromium.org
Sky, PTAL!
https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... File components/sync/model_impl/shared_model_type_processor.cc (left): https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:153: is_metadata_loaded_ = true; Glad to see this removed. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... File components/sync/model_impl/shared_model_type_processor.cc (right): https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:55: DCHECK(entities_.empty()); These can be above the if (start_error_) check, right? Can you move them? https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:60: is_initial_pending_data_loaded_ = true; What do you think of changing these two booleans to track if we're currently waiting for the model to give us metadata or data. The metadata one starts true, data starts false. This particular line goes away, and the pending data is only flipped when we actually care about it. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:87: base::Bind(&SharedModelTypeProcessor::OnInitialPendingDataLoaded, Would be great if we exposed this as a OnceCallback eventually. Not necessary to do in this CL though. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:98: // and ConnectIfReady's preconditions can't be passed twice. "ConnectIfReady's preconditions can't be passed twice" seems false, if you ignore the DCHECK. The ConnectPreconditionsMet() has the word preconditions in it, DCHECK does not. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:117: DCHECK(start_callback_); Why don't we just move this into the actual preconditions check and make this function idempotent? https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:135: // Changes can be handled correctly even before pending data is loaded. Great comment. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:164: // If both model and sync are ready, then |start_callback_| was already Ugh. This is messy. Like, why do we need to differentiate between a start error and non-start error? Would be nice if we had a component that would manager errors for us. class ErrorHandler { bool IsInError(); void AddError(ModelError); void SetReporter(Callback<void(ModelError)>); } Regardless of our current state, we can just give errors to this thing. If it doesn't have a reporter yet, it will queue them up in a list. It will always be able to tell us if we've ever hit an error or not. And when we give it an error reporting mechanism, then it will send the old ones, and be able to send new ones. WDYT? https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:594: DCHECK(!is_initial_pending_data_loaded_); Can you move this above the if (start_error_) check? https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... File components/sync/model_impl/shared_model_type_processor.h (left): https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.h:76: using EntityMap = I like how "cleaned up" means delete them all.
The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run
PTAL, newest patch has a lot of changes. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... File components/sync/model_impl/shared_model_type_processor.cc (left): https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:153: is_metadata_loaded_ = true; On 2017/01/18 17:31:38, skym wrote: > Glad to see this removed. Me too! https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... File components/sync/model_impl/shared_model_type_processor.cc (right): https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:55: DCHECK(entities_.empty()); On 2017/01/18 17:31:38, skym wrote: > These can be above the if (start_error_) check, right? Can you move them? Oh yeah, good catch. Done. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:60: is_initial_pending_data_loaded_ = true; On 2017/01/18 17:31:38, skym wrote: > What do you think of changing these two booleans to track if we're currently > waiting for the model to give us metadata or data. The metadata one starts true, > data starts false. This particular line goes away, and the pending data is only > flipped when we actually care about it. Love it, done. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:87: base::Bind(&SharedModelTypeProcessor::OnInitialPendingDataLoaded, On 2017/01/18 17:31:38, skym wrote: > Would be great if we exposed this as a OnceCallback eventually. Not necessary to > do in this CL though. Acknowledged. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:98: // and ConnectIfReady's preconditions can't be passed twice. On 2017/01/18 17:31:38, skym wrote: > "ConnectIfReady's preconditions can't be passed twice" seems false, if you > ignore the DCHECK. The ConnectPreconditionsMet() has the word preconditions in > it, DCHECK does not. n/a in most recent patch. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:117: DCHECK(start_callback_); On 2017/01/18 17:31:38, skym wrote: > Why don't we just move this into the actual preconditions check and make this > function idempotent? Done. We shouldn't be crashing on anything we don't have to. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:135: // Changes can be handled correctly even before pending data is loaded. On 2017/01/18 17:31:38, skym wrote: > Great comment. Acknowledged. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:164: // If both model and sync are ready, then |start_callback_| was already On 2017/01/18 17:31:38, skym wrote: > Ugh. This is messy. Like, why do we need to differentiate between a start error > and non-start error? > > Would be nice if we had a component that would manager errors for us. > > class ErrorHandler { > bool IsInError(); > void AddError(ModelError); > void SetReporter(Callback<void(ModelError)>); > } > > Regardless of our current state, we can just give errors to this thing. If it > doesn't have a reporter yet, it will queue them up in a list. It will always be > able to tell us if we've ever hit an error or not. And when we give it an error > reporting mechanism, then it will send the old ones, and be able to send new > ones. WDYT? As discussed offline, I'm not a fan of this approach. I don't think it would actually make things cleaner. But, take a look at the new patch and let me know what you think! https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.cc:594: DCHECK(!is_initial_pending_data_loaded_); On 2017/01/18 17:31:38, skym wrote: > Can you move this above the if (start_error_) check? Done. https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... File components/sync/model_impl/shared_model_type_processor.h (left): https://codereview.chromium.org/2642493003/diff/1/components/sync/model_impl/... components/sync/model_impl/shared_model_type_processor.h:76: using EntityMap = On 2017/01/18 17:31:39, skym wrote: > I like how "cleaned up" means delete them all. No code is best code!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Sync] Clean up SMTP startup flow. - The start_error_ is no longer cleared and is the permanent indicator that the model is in an error state. - ConnectIfReady is not called if GetData for pending changes was called; this was the source of the crash in 679657. - Regression test added for the above issue. - std::move() the key lists for GetData calls for efficiency. - Cleaned up DCHECKS a bit and used ASSERT or EXPECT in tests instead. - Remove EntityMap and UpdateMap typedefs in the SMTP header. BUG=673883,679657 ========== to ========== [Sync] Clean up SMTP startup flow. - start_error_ renamed model_error_, is always set on error, never cleared, and is the permanent indicator that the model is in an error state. - ConnectIfReady now no-ops if called more than once. This could happen if the get pending data callback was called synchronously and was the source of the crash in 679657. - Regression test added for the above issue. - std::move() the key lists for GetData calls for efficiency. - Cleaned up DCHECKS a bit and used ASSERT or EXPECT in tests instead. - Remove EntityMap and UpdateMap typedefs in the SMTP header. BUG=673883,679657 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2642493003/diff/20001/components/sync/model_i... File components/sync/model_impl/shared_model_type_processor.cc (right): https://codereview.chromium.org/2642493003/diff/20001/components/sync/model_i... components/sync/model_impl/shared_model_type_processor.cc:126: DCHECK(!waiting_for_metadata_); Is this really true now that you don't set is_metadata_loaded_ = true on error? https://codereview.chromium.org/2642493003/diff/20001/components/sync/model_i... components/sync/model_impl/shared_model_type_processor.cc:151: // Sync is ready but the model wasn't yet. These comments are hard to write because ConnectIfReady() has such a bizarre job. What do you think of something like: // Tell Sync we're in an error state instead of connecting. https://codereview.chromium.org/2642493003/diff/20001/components/sync/model_i... components/sync/model_impl/shared_model_type_processor.cc:154: // Sync and the model were both already ready. What do you think of something like: // We've already connected, instead of going through ConnectIfReady(), just send the error to Sync.
https://codereview.chromium.org/2642493003/diff/20001/components/sync/model_i... File components/sync/model_impl/shared_model_type_processor.cc (right): https://codereview.chromium.org/2642493003/diff/20001/components/sync/model_i... components/sync/model_impl/shared_model_type_processor.cc:126: DCHECK(!waiting_for_metadata_); On 2017/01/18 21:24:59, skym wrote: > Is this really true now that you don't set is_metadata_loaded_ = true on error? Probably not. Removing it. https://codereview.chromium.org/2642493003/diff/20001/components/sync/model_i... components/sync/model_impl/shared_model_type_processor.cc:151: // Sync is ready but the model wasn't yet. On 2017/01/18 21:24:59, skym wrote: > These comments are hard to write because ConnectIfReady() has such a bizarre > job. What do you think of something like: > > // Tell Sync we're in an error state instead of connecting. Doing: // Tell sync about the error instead of connecting. https://codereview.chromium.org/2642493003/diff/20001/components/sync/model_i... components/sync/model_impl/shared_model_type_processor.cc:154: // Sync and the model were both already ready. On 2017/01/18 21:24:59, skym wrote: > What do you think of something like: > > // We've already connected, instead of going through ConnectIfReady(), just send > the error to Sync. Doing: // Connecting was already initiated; just tell sync about the error instead of going through ConnectIfReady().
The CQ bit was checked by maxbogue@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org Link to the patchset: https://codereview.chromium.org/2642493003/#ps40001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484782319534230,
"parent_rev": "40f6edc4362a890afb934c287ae90a7125e5b7d7", "commit_rev":
"d462fc419f40c24915a6c32cea50ecb3c5273f65"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] Clean up SMTP startup flow. - start_error_ renamed model_error_, is always set on error, never cleared, and is the permanent indicator that the model is in an error state. - ConnectIfReady now no-ops if called more than once. This could happen if the get pending data callback was called synchronously and was the source of the crash in 679657. - Regression test added for the above issue. - std::move() the key lists for GetData calls for efficiency. - Cleaned up DCHECKS a bit and used ASSERT or EXPECT in tests instead. - Remove EntityMap and UpdateMap typedefs in the SMTP header. BUG=673883,679657 ========== to ========== [Sync] Clean up SMTP startup flow. - start_error_ renamed model_error_, is always set on error, never cleared, and is the permanent indicator that the model is in an error state. - ConnectIfReady now no-ops if called more than once. This could happen if the get pending data callback was called synchronously and was the source of the crash in 679657. - Regression test added for the above issue. - std::move() the key lists for GetData calls for efficiency. - Cleaned up DCHECKS a bit and used ASSERT or EXPECT in tests instead. - Remove EntityMap and UpdateMap typedefs in the SMTP header. BUG=673883,679657 Review-Url: https://codereview.chromium.org/2642493003 Cr-Commit-Position: refs/heads/master@{#444561} Committed: https://chromium.googlesource.com/chromium/src/+/d462fc419f40c24915a6c32cea50... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d462fc419f40c24915a6c32cea50... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
