Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2958183002:
Add sequencing comment to PpdCacheImpl. (Closed)
DescriptionAdd sequencing comment to PpdCacheImpl.
We currently assume that any given file is only Stored once. This
allows for Find and Store to run in different sequences at different
priorities. Document this assumption in case this changes and a subtle
bug gets introduced.
BUG=734279
Review-Url: https://codereview.chromium.org/2958183002
Cr-Commit-Position: refs/heads/master@{#484401}
Committed: https://chromium.googlesource.com/chromium/src/+/e85631c79cebfbf8c6004ac54eab93d9d00f4647
Patch Set 1 #
Total comments: 2
Patch Set 2 : update comment #Messages
Total messages: 17 (7 generated)
skau@chromium.org changed reviewers: + justincarlson@chromium.org
https://codereview.chromium.org/2958183002/diff/1/chromeos/printing/ppd_cache.cc File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2958183002/diff/1/chromeos/printing/ppd_cache... chromeos/printing/ppd_cache.cc:123: // assumption is invalidated, the sequencing will require revisiting. The more I think about it, the more I'm convinced that we've gone a bit wrong here. I think we don't have any real guarantee here from sequencing. We could have concurrent stores and find for the same file going on within a single PpdCache -- we could Find(foo) - miss Store(foo) - (kicks off actual disk io to store_task_runner_) Find(foo) - (disk io on fetch_task_runner_) hit on file, but file is not completely written yet, so get incomplete contents. Furthermore, we could have *multiple* ppd providers instantiated at the same time, which means we don't actually get any guarantees from things being in the same object -- two objects could be doing entirely different things on the same filesystem. I think probably the checksum mechanism is good enough to make this all ok, but I don't think this comment is right.
Updated comment. I don't think we need to mention checksums but let me know if you think that would be helpful. https://codereview.chromium.org/2958183002/diff/1/chromeos/printing/ppd_cache.cc File chromeos/printing/ppd_cache.cc (right): https://codereview.chromium.org/2958183002/diff/1/chromeos/printing/ppd_cache... chromeos/printing/ppd_cache.cc:123: // assumption is invalidated, the sequencing will require revisiting. On 2017/06/29 18:47:09, Carlson wrote: > The more I think about it, the more I'm convinced that we've gone a bit wrong > here. > > I think we don't have any real guarantee here from sequencing. We could have > concurrent stores and find for the same file going on within a single PpdCache > -- we could > > Find(foo) - miss > Store(foo) - (kicks off actual disk io to store_task_runner_) > Find(foo) - (disk io on fetch_task_runner_) hit on file, but file is not > completely written yet, so get incomplete contents. > > Furthermore, we could have *multiple* ppd providers instantiated at the same > time, which means we don't actually get any guarantees from things being in the > same object -- two objects could be doing entirely different things on the same > filesystem. > > I think probably the checksum mechanism is good enough to make this all ok, but > I don't think this comment is right. Okay. It's updated. Really, what we're saying is that the sequences don't matter, we actually need to synchronize with the file system. I say a little more about expected behavior instead of assumptions now.
lgtm
The CQ bit was checked by skau@chromium.org
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
skau@chromium.org changed reviewers: + xdai@google.com
xdai@: Can you approve this? I need lgtm from a committer.
lgtm
The CQ bit was checked by skau@chromium.org
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": 20001, "attempt_start_ts": 1499295626876580,
"parent_rev": "792f3cdfe83c2df42b0a78eba9e59d4c016814d2", "commit_rev":
"e85631c79cebfbf8c6004ac54eab93d9d00f4647"}
Message was sent while issue was closed.
Description was changed from ========== Add sequencing comment to PpdCacheImpl. We currently assume that any given file is only Stored once. This allows for Find and Store to run in different sequences at different priorities. Document this assumption in case this changes and a subtle bug gets introduced. BUG=734279 ========== to ========== Add sequencing comment to PpdCacheImpl. We currently assume that any given file is only Stored once. This allows for Find and Store to run in different sequences at different priorities. Document this assumption in case this changes and a subtle bug gets introduced. BUG=734279 Review-Url: https://codereview.chromium.org/2958183002 Cr-Commit-Position: refs/heads/master@{#484401} Committed: https://chromium.googlesource.com/chromium/src/+/e85631c79cebfbf8c6004ac54eab... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e85631c79cebfbf8c6004ac54eab... |
