Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2476073003:
Update PpdProvider threading model. (Closed)
|
Created:
4 years, 1 month ago by Carlson Modified:
4 years, 1 month ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, oshima+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate PpdProvider threading model.
We are crashing the browser right now because PpdProvider does disk
operations (via PpdCache), and those are invoked on the UI thread. Fix
this by making PpdProvider take an explicit task runner on which to do
disk operations and pull them off the UI thread.
BUG=662626
Committed: https://crrev.com/c31c58ba48632b1713cde7c0e83483595907fb20
Cr-Commit-Position: refs/heads/master@{#432062}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address skau comments #Patch Set 3 : Add more logging, add bypass for manufacturer issue #
Total comments: 12
Patch Set 4 : Shift PpdProvider fetcher states into fetcher delegates. #Patch Set 5 : Rework threading per stevenjb@'s comments #Patch Set 6 : Fix some comment typos. #
Total comments: 27
Patch Set 7 : Address more stevenjb comments, mostly naming. #Patch Set 8 : Fix two comments I missed. #Patch Set 9 : Change CHECK to DCHECK #
Messages
Total messages: 38 (15 generated)
Description was changed from ========== Update PpdProvider threading model. We are crashing the browser right now because PpdProvider does disk operations (via PpdCache), and those are invoked on the UI thread. Fix this by making PpdProvider take an explicit task runner on which to do disk operations and pull them off the UI thread. BUG=662626 ========== to ========== Update PpdProvider threading model. We are crashing the browser right now because PpdProvider does disk operations (via PpdCache), and those are invoked on the UI thread. Fix this by making PpdProvider take an explicit task runner on which to do disk operations and pull them off the UI thread. BUG=662626 ==========
justincarlson@chromium.org changed reviewers: + skau@chromium.org
Sean, when you get a chance can you take a look? I can't reproduce the crash with this patch, so I think we're in good shape.
It looks like the task runners are not released until we run another resolve or another query. Should we be concerned about holding those references? https://codereview.chromium.org/2476073003/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2476073003/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:88: .get(), Why are you using the raw pointer instead of the smart pointer? https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:81: CHECK(resolve_fetcher_ == nullptr) Can you add that you're using resolve_fetcher_ == null to check for running fetches on the member variable comment. https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.h:80: // to |url_context_getter| and |disk_task_runner| are taken. If you're taking a reference to these objects, you should pass the scoped_refptr https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md
I patched the CL and I may have found a bug. https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider.cc (left): https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:122: void QueryAvailable(const QueryAvailableCallback& cb) override { It looks like model and manufacturer is being queried concurrently and causing a crash. Can you look into it?
https://codereview.chromium.org/2476073003/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc (right): https://codereview.chromium.org/2476073003/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc:88: .get(), On 2016/11/09 00:01:09, skau wrote: > Why are you using the raw pointer instead of the smart pointer? Changed to scoped_refptr (see other comment) https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider.cc (left): https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:122: void QueryAvailable(const QueryAvailableCallback& cb) override { On 2016/11/10 02:38:41, skau wrote: > It looks like model and manufacturer is being queried concurrently and causing a > crash. Can you look into it? We chatted offline about this. Being fixed not in this cl. https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.cc:81: CHECK(resolve_fetcher_ == nullptr) On 2016/11/09 00:01:09, skau wrote: > Can you add that you're using resolve_fetcher_ == null to check for running > fetches on the member variable comment. Done. https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2476073003/diff/1/chromeos/printing/ppd_provi... chromeos/printing/ppd_provider.h:80: // to |url_context_getter| and |disk_task_runner| are taken. On 2016/11/09 00:01:09, skau wrote: > If you're taking a reference to these objects, you should pass the scoped_refptr > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Ugh, I had found (apparently older) advice that said just the opposite. Changed back.
lgtm
The CQ bit was checked by justincarlson@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.
I missed a few things you commented on in general. Addressed them here. I also added a workaround for the manufacturer/model concurrent query problem that doesn't need to hit the javascript. PTAL?
justincarlson@chromium.org changed reviewers: + hcarmona@chromium.org
Hector, can I ask you to look over the cups_printers_handler changes? Thanks, -J
hcarmona@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb@ knows more about chromeos and might be a better reviewer for this CL.
hcarmona@chromium.org changed reviewers: - hcarmona@chromium.org
So, I proposed a fairly significant change, but given the complexity of the code here, and the fact that there is already a thread related bug, I'm not convinced that adding 3 TaskRunner members is the best solution. https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:84: resolve_done_task_runner_ = base::SequencedTaskRunnerHandle::Get(); There is a lot of complexity here. I am wondering if we can simplify it some using existing tools. https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:87: weak_factory_.GetWeakPtr(), ppd_reference, cb)); Here we would normally use TaskRunner::PostTaskAndReplyWithResult. https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:105: FROM_HERE, base::Bind(cb, PpdProvider::SUCCESS, tmp.value())); We would need to pass a unique_ptr<FilePath> to this, set it, and return PpdProvider::SUCCESS here. https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:117: FROM_HERE, base::Bind(cb, PpdProvider::NOT_FOUND, base::FilePath())); This would return PpdProvider::NOT_FOUND. https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:120: It looks like the rest of this can be safely called on the primary task runner, so here we could return "success" with an empty file, so the rest of this would happen in the "Reply" method. https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:126: net::URLFetcher::GET, &forwarding_delegate_); Here, instead of using a forwarding_delegate_ member, construct a new delegate for each request and pass |cb| and |ppd_reference| to it. The delegate can store that task runner it was created on (base::SequencedTaskRunnerHandle::Get) which would be the same task runner that Resolve() was called on, and ForwardingURLFetcherDelegate::OnURLFetchComplete() can post owner_->OnURLFetchComplete() on that task runner, with |cb| and |ppd_reference| as arguments. That way, instead of storing state within PpdProviderImpl (which is effectively a global), we store the state in bindings and ForwardingURLFetcherDelegate. This will make it easier for other developers to follow what is actually part of the global state, and what is part of the current request. Even if we never need to surpport more than one request at a time, this will make the code somewhat easier to follow. https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.h:84: scoped_refptr<base::SequencedTaskRunner> disk_task_runner, nit: file_task_runner is a more common term.
Thanks for the ideas/comments! Unreplied comments are all related to the PostTaskAndReplyWithResult suggestion which needs resolution at a high level before those pieces can be addressed. https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:84: resolve_done_task_runner_ = base::SequencedTaskRunnerHandle::Get(); On 2016/11/11 19:26:43, stevenjb wrote: > There is a lot of complexity here. I am wondering if we can simplify it some > using existing tools. I appreciate you taking the time to look carefully for stuff like this. I'm still finding my way around this codebase and likely doing many things in rather suboptimal ways. https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:87: weak_factory_.GetWeakPtr(), ppd_reference, cb)); On 2016/11/11 19:26:43, stevenjb wrote: > Here we would normally use TaskRunner::PostTaskAndReplyWithResult. > I'm not sure that works for what we want here. Or, rather, I think it doesn't work, but would be happy to find out otherwise. If I use PostTaskAndReplyWithResult, then I'm going to end up with the callback on the disk thread, right? I thought it was idiomatic in Chrome to have the callback for an API like this be invoked on the same thread, or at least same sequence, as the API function. I don't see how to do that with PostTaskAndReply* What am I missing? https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:126: net::URLFetcher::GET, &forwarding_delegate_); On 2016/11/11 19:26:43, stevenjb wrote: > Here, instead of using a forwarding_delegate_ member, construct a new delegate > for each request and pass |cb| and |ppd_reference| to it. The delegate can store > that task runner it was created on (base::SequencedTaskRunnerHandle::Get) which > would be the same task runner that Resolve() was called on, and > ForwardingURLFetcherDelegate::OnURLFetchComplete() can post > owner_->OnURLFetchComplete() on that task runner, with |cb| and |ppd_reference| > as arguments. > > That way, instead of storing state within PpdProviderImpl (which is effectively > a global), we store the state in bindings and ForwardingURLFetcherDelegate. This > will make it easier for other developers to follow what is actually part of the > global state, and what is part of the current request. Even if we never need to > surpport more than one request at a time, this will make the code somewhat > easier to follow. I spent a while going down this path (because it does seem like a good idea), but the lifetime of a per-request delegate object is problematic -- I don't see a good way to ensure that it gets cleaned up. It can't be self-deleting because we have no guarantee that OnURLFetchComplete() (or any other delegate method) will ever be called. Did you have an idea here I'm missing? I can still shift some stuff like the query_task_runner_, query_done_callback_ and similar things into the delegate object, but if we have to keep the ownership in PpdProvider, we still end up with this sort of pseudo-global state around the fetch. I'm happy to do that, just not sure it's as worth it without the other benefits. Let me know what you think, or if you have another idea.
https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:87: weak_factory_.GetWeakPtr(), ppd_reference, cb)); On 2016/11/11 23:06:50, Carlson wrote: > On 2016/11/11 19:26:43, stevenjb wrote: > > Here we would normally use TaskRunner::PostTaskAndReplyWithResult. > > > > I'm not sure that works for what we want here. Or, rather, I think it doesn't > work, but would be happy to find out otherwise. > > If I use PostTaskAndReplyWithResult, then I'm going to end up with the callback > on the disk thread, right? I thought it was idiomatic in Chrome to have the > callback for an API like this be invoked on the same thread, or at least same > sequence, as the API function. I don't see how to do that with > PostTaskAndReply* > > What am I missing? See the documentaiton for TaskRunner::PostTaskAndReply, that's exactly what it does: // Posts |task| on the current TaskRunner. On completion, |reply| // is posted to the thread that called PostTaskAndReply()... https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... chromeos/printing/ppd_provider.cc:126: net::URLFetcher::GET, &forwarding_delegate_); On 2016/11/11 23:06:50, Carlson wrote: > On 2016/11/11 19:26:43, stevenjb wrote: > > Here, instead of using a forwarding_delegate_ member, construct a new delegate > > for each request and pass |cb| and |ppd_reference| to it. The delegate can > store > > that task runner it was created on (base::SequencedTaskRunnerHandle::Get) > which > > would be the same task runner that Resolve() was called on, and > > ForwardingURLFetcherDelegate::OnURLFetchComplete() can post > > owner_->OnURLFetchComplete() on that task runner, with |cb| and > |ppd_reference| > > as arguments. > > > > That way, instead of storing state within PpdProviderImpl (which is > effectively > > a global), we store the state in bindings and ForwardingURLFetcherDelegate. > This > > will make it easier for other developers to follow what is actually part of > the > > global state, and what is part of the current request. Even if we never need > to > > surpport more than one request at a time, this will make the code somewhat > > easier to follow. > > I spent a while going down this path (because it does seem like a good idea), > but the lifetime of a per-request delegate object is problematic -- I don't see > a good way to ensure that it gets cleaned up. It can't be self-deleting because > we have no guarantee that OnURLFetchComplete() (or any other delegate method) > will ever be called. > > > Did you have an idea here I'm missing? > > I can still shift some stuff like the query_task_runner_, query_done_callback_ > and similar things into the delegate object, but if we have to keep the > ownership in PpdProvider, we still end up with this sort of pseudo-global state > around the fetch. > > I'm happy to do that, just not sure it's as worth it without the other benefits. > Let me know what you think, or if you have another idea. So, for now at least, we can continue to have a single resolve_fetcher_, and can continue to check above that resolve_fetcher_ is null. Anywhere we reset it (OnResolveFetchComplete, AbortResolve()), we should also reset the associated delegate. We could also just keep the current model but pass the parameters to the single delegate instance each time. Either way, I think that wrapping the members associated with the current request in the delegate still makes sense. It at least makes it a bit more clear that those members are tied to the request.
On 2016/11/11 23:25:37, stevenjb wrote: > https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... > File chromeos/printing/ppd_provider.cc (right): > > https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... > chromeos/printing/ppd_provider.cc:87: weak_factory_.GetWeakPtr(), ppd_reference, > cb)); > On 2016/11/11 23:06:50, Carlson wrote: > > On 2016/11/11 19:26:43, stevenjb wrote: > > > Here we would normally use TaskRunner::PostTaskAndReplyWithResult. > > > > > > > I'm not sure that works for what we want here. Or, rather, I think it doesn't > > work, but would be happy to find out otherwise. > > > > If I use PostTaskAndReplyWithResult, then I'm going to end up with the > callback > > on the disk thread, right? I thought it was idiomatic in Chrome to have the > > callback for an API like this be invoked on the same thread, or at least same > > sequence, as the API function. I don't see how to do that with > > PostTaskAndReply* > > > > What am I missing? > > See the documentaiton for TaskRunner::PostTaskAndReply, that's exactly what it > does: > > // Posts |task| on the current TaskRunner. On completion, |reply| > // is posted to the thread that called PostTaskAndReply()... Ok, cool, I see how this makes a lot of sense. However, I think we still have some obstacles here, because the tasks being posted here are themselves (sometimes) callers of callback-based apis. For example, ResolveImpl() can determine the result when it hits in the local cache, but when we have to actually go hit the server, we don't know the result until OnResolveFetchComplete() is called. So ResolveImpl *can't* return a result directly. We could shoehorn this into the right sort of shape by making ResolveImpl and QueryAvailableImpl synchronous (by stalling the calling thread until the URL fetch completes), but I would think that would be both wasteful and hard to reason about w.r.t. possible deadlock. Or am I still missing something? > > https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... > chromeos/printing/ppd_provider.cc:126: net::URLFetcher::GET, > &forwarding_delegate_); > On 2016/11/11 23:06:50, Carlson wrote: > > On 2016/11/11 19:26:43, stevenjb wrote: > > > Here, instead of using a forwarding_delegate_ member, construct a new > delegate > > > for each request and pass |cb| and |ppd_reference| to it. The delegate can > > store > > > that task runner it was created on (base::SequencedTaskRunnerHandle::Get) > > which > > > would be the same task runner that Resolve() was called on, and > > > ForwardingURLFetcherDelegate::OnURLFetchComplete() can post > > > owner_->OnURLFetchComplete() on that task runner, with |cb| and > > |ppd_reference| > > > as arguments. > > > > > > That way, instead of storing state within PpdProviderImpl (which is > > effectively > > > a global), we store the state in bindings and ForwardingURLFetcherDelegate. > > This > > > will make it easier for other developers to follow what is actually part of > > the > > > global state, and what is part of the current request. Even if we never need > > to > > > surpport more than one request at a time, this will make the code somewhat > > > easier to follow. > > > > I spent a while going down this path (because it does seem like a good idea), > > but the lifetime of a per-request delegate object is problematic -- I don't > see > > a good way to ensure that it gets cleaned up. It can't be self-deleting > because > > we have no guarantee that OnURLFetchComplete() (or any other delegate method) > > will ever be called. > > > > > > Did you have an idea here I'm missing? > > > > I can still shift some stuff like the query_task_runner_, query_done_callback_ > > and similar things into the delegate object, but if we have to keep the > > ownership in PpdProvider, we still end up with this sort of pseudo-global > state > > around the fetch. > > > > I'm happy to do that, just not sure it's as worth it without the other > benefits. > > Let me know what you think, or if you have another idea. > > So, for now at least, we can continue to have a single resolve_fetcher_, and can > continue to check above that resolve_fetcher_ is null. Anywhere we reset it > (OnResolveFetchComplete, AbortResolve()), we should also reset the associated > delegate. > > We could also just keep the current model but pass the parameters to the single > delegate instance each time. > > Either way, I think that wrapping the members associated with the current > request in the delegate still makes sense. It at least makes it a bit more clear > that those members are tied to the request. Did this. To make it a little more orthogonal, I converted the Delegate class to just be a thin wrapper around a callback, otherwise I needed to either do separate delegate classes for each kind of url fetch, or do a weird pseudo-union of fields in the object; it seems cleaner just to carry the state in the passed callback. Let me know what you think.
On 2016/11/12 00:54:40, Carlson wrote: > On 2016/11/11 23:25:37, stevenjb wrote: > > > https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... > > File chromeos/printing/ppd_provider.cc (right): > > > > > https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... > > chromeos/printing/ppd_provider.cc:87: weak_factory_.GetWeakPtr(), > ppd_reference, > > cb)); > > On 2016/11/11 23:06:50, Carlson wrote: > > > On 2016/11/11 19:26:43, stevenjb wrote: > > > > Here we would normally use TaskRunner::PostTaskAndReplyWithResult. > > > > > > > > > > I'm not sure that works for what we want here. Or, rather, I think it > doesn't > > > work, but would be happy to find out otherwise. > > > > > > If I use PostTaskAndReplyWithResult, then I'm going to end up with the > > callback > > > on the disk thread, right? I thought it was idiomatic in Chrome to have the > > > callback for an API like this be invoked on the same thread, or at least > same > > > sequence, as the API function. I don't see how to do that with > > > PostTaskAndReply* > > > > > > What am I missing? > > > > See the documentaiton for TaskRunner::PostTaskAndReply, that's exactly what it > > does: > > > > // Posts |task| on the current TaskRunner. On completion, |reply| > > // is posted to the thread that called PostTaskAndReply()... > > Ok, cool, I see how this makes a lot of sense. > > However, I think we still have some obstacles here, because the tasks being > posted here are themselves (sometimes) callers of callback-based apis. For > example, ResolveImpl() can determine the result when it hits in the local cache, > but when we have to actually go hit the server, we don't know the result until > OnResolveFetchComplete() is called. So ResolveImpl *can't* return a result > directly. > > We could shoehorn this into the right sort of shape by making ResolveImpl and > QueryAvailableImpl synchronous (by stalling the calling thread until the URL > fetch completes), but I would think that would be both wasteful and hard to > reason about w.r.t. possible deadlock. > > Or am I still missing something? > > > > > > https://codereview.chromium.org/2476073003/diff/40001/chromeos/printing/ppd_p... > > chromeos/printing/ppd_provider.cc:126: net::URLFetcher::GET, > > &forwarding_delegate_); > > On 2016/11/11 23:06:50, Carlson wrote: > > > On 2016/11/11 19:26:43, stevenjb wrote: > > > > Here, instead of using a forwarding_delegate_ member, construct a new > > delegate > > > > for each request and pass |cb| and |ppd_reference| to it. The delegate can > > > store > > > > that task runner it was created on (base::SequencedTaskRunnerHandle::Get) > > > which > > > > would be the same task runner that Resolve() was called on, and > > > > ForwardingURLFetcherDelegate::OnURLFetchComplete() can post > > > > owner_->OnURLFetchComplete() on that task runner, with |cb| and > > > |ppd_reference| > > > > as arguments. > > > > > > > > That way, instead of storing state within PpdProviderImpl (which is > > > effectively > > > > a global), we store the state in bindings and > ForwardingURLFetcherDelegate. > > > This > > > > will make it easier for other developers to follow what is actually part > of > > > the > > > > global state, and what is part of the current request. Even if we never > need > > > to > > > > surpport more than one request at a time, this will make the code somewhat > > > > easier to follow. > > > > > > I spent a while going down this path (because it does seem like a good > idea), > > > but the lifetime of a per-request delegate object is problematic -- I don't > > see > > > a good way to ensure that it gets cleaned up. It can't be self-deleting > > because > > > we have no guarantee that OnURLFetchComplete() (or any other delegate > method) > > > will ever be called. > > > > > > > > > Did you have an idea here I'm missing? > > > > > > I can still shift some stuff like the query_task_runner_, > query_done_callback_ > > > and similar things into the delegate object, but if we have to keep the > > > ownership in PpdProvider, we still end up with this sort of pseudo-global > > state > > > around the fetch. > > > > > > I'm happy to do that, just not sure it's as worth it without the other > > benefits. > > > Let me know what you think, or if you have another idea. > > > > So, for now at least, we can continue to have a single resolve_fetcher_, and > can > > continue to check above that resolve_fetcher_ is null. Anywhere we reset it > > (OnResolveFetchComplete, AbortResolve()), we should also reset the associated > > delegate. > > > > We could also just keep the current model but pass the parameters to the > single > > delegate instance each time. > > > > Either way, I think that wrapping the members associated with the current > > request in the delegate still makes sense. It at least makes it a bit more > clear > > that those members are tied to the request. > > Did this. To make it a little more orthogonal, I converted the Delegate class > to just be a thin wrapper around a callback, otherwise I needed to either do > separate delegate classes for each kind of url fetch, or do a weird pseudo-union > of fields in the object; it seems cleaner just to carry the state in the passed > callback. Let me know what you think. Oh, forgot to also mention this delta rebases on top of some other changes that went in recently. Sorry about the extra diffspam. -J
I won't have a chance to re-review this again until Monday, however in
regard to your questions / concerns:
* We should not do any blocking, you are correct on that account.
* Usually, the code can be structured such that the primary thread (i.e.
the thread that invoked the request, usually the UI thread) runs the
overall state machine, with the io task runner handling blocking requests.
We try to avoid having a method running on a blocking task runner (e.g. a
file request) spawn another blocking task (e.g. a URLFetcher request).
In fact, re-scanning the code, it is not clear to me whether calling
URLFetcher::Create on the blocking task runner is desired or even valid.
So to rephrase my suggestion:
1. Rename disk_task_runner_ to blocking_ or io_task_runner since we're
(maybe?) using it for non disk operations (and either way we really
shouldn't care what it's for beyond "slow blocking tasks").
2. Break up ResolveImpl into (a) a method that does file operations, and
(b) a method that calls URLFetcher::Create. Even if both need to be done on
the blocking thread (and we should understand whether or not that is true),
it will be easier to follow the code if those are separate tasks. Rename
these methods something like ResloveCachedPpd and ResolvePppUrlFetch (or
whatever makes sense).
3. We really ought to be able to use PostTaskAndReply{WithResponse} instead
of passing a task runner to the methods. That's pretty much what it is for.
On Fri, Nov 11, 2016 at 4:59 PM, <justincarlson@google.com> wrote:
> On 2016/11/12 00:54:40, Carlson wrote:
> > On 2016/11/11 23:25:37, stevenjb wrote:
> > >
> >
> https://codereview.chromium.org/2476073003/diff/40001/
> chromeos/printing/ppd_provider.cc
> > > File chromeos/printing/ppd_provider.cc (right):
> > >
> > >
> >
> https://codereview.chromium.org/2476073003/diff/40001/
> chromeos/printing/ppd_provider.cc#newcode87
> > > chromeos/printing/ppd_provider.cc:87: weak_factory_.GetWeakPtr(),
> > ppd_reference,
> > > cb));
> > > On 2016/11/11 23:06:50, Carlson wrote:
> > > > On 2016/11/11 19:26:43, stevenjb wrote:
> > > > > Here we would normally use TaskRunner::PostTaskAndReplyWithResult.
> > > > >
> > > >
> > > > I'm not sure that works for what we want here. Or, rather, I think it
> > doesn't
> > > > work, but would be happy to find out otherwise.
> > > >
> > > > If I use PostTaskAndReplyWithResult, then I'm going to end up with
> the
> > > callback
> > > > on the disk thread, right? I thought it was idiomatic in Chrome to
> have
> the
> > > > callback for an API like this be invoked on the same thread, or at
> least
> > same
> > > > sequence, as the API function. I don't see how to do that with
> > > > PostTaskAndReply*
> > > >
> > > > What am I missing?
> > >
> > > See the documentaiton for TaskRunner::PostTaskAndReply, that's exactly
> what
> it
> > > does:
> > >
> > > // Posts |task| on the current TaskRunner. On completion, |reply|
> > > // is posted to the thread that called PostTaskAndReply()...
> >
> > Ok, cool, I see how this makes a lot of sense.
> >
> > However, I think we still have some obstacles here, because the tasks
> being
> > posted here are themselves (sometimes) callers of callback-based apis.
> For
> > example, ResolveImpl() can determine the result when it hits in the local
> cache,
> > but when we have to actually go hit the server, we don't know the result
> until
> > OnResolveFetchComplete() is called. So ResolveImpl *can't* return a
> result
> > directly.
> >
> > We could shoehorn this into the right sort of shape by making
> ResolveImpl and
> > QueryAvailableImpl synchronous (by stalling the calling thread until the
> URL
> > fetch completes), but I would think that would be both wasteful and hard
> to
> > reason about w.r.t. possible deadlock.
> >
> > Or am I still missing something?
> >
> > >
> > >
> >
> https://codereview.chromium.org/2476073003/diff/40001/
> chromeos/printing/ppd_provider.cc#newcode126
> > > chromeos/printing/ppd_provider.cc:126: net::URLFetcher::GET,
> > > &forwarding_delegate_);
> > > On 2016/11/11 23:06:50, Carlson wrote:
> > > > On 2016/11/11 19:26:43, stevenjb wrote:
> > > > > Here, instead of using a forwarding_delegate_ member, construct a
> new
> > > delegate
> > > > > for each request and pass |cb| and |ppd_reference| to it. The
> delegate
> can
> > > > store
> > > > > that task runner it was created on
> (base::SequencedTaskRunnerHandle::Get)
> > > > which
> > > > > would be the same task runner that Resolve() was called on, and
> > > > > ForwardingURLFetcherDelegate::OnURLFetchComplete() can post
> > > > > owner_->OnURLFetchComplete() on that task runner, with |cb| and
> > > > |ppd_reference|
> > > > > as arguments.
> > > > >
> > > > > That way, instead of storing state within PpdProviderImpl (which is
> > > > effectively
> > > > > a global), we store the state in bindings and
> > ForwardingURLFetcherDelegate.
> > > > This
> > > > > will make it easier for other developers to follow what is
> actually part
> > of
> > > > the
> > > > > global state, and what is part of the current request. Even if we
> never
> > need
> > > > to
> > > > > surpport more than one request at a time, this will make the code
> somewhat
> > > > > easier to follow.
> > > >
> > > > I spent a while going down this path (because it does seem like a
> good
> > idea),
> > > > but the lifetime of a per-request delegate object is problematic -- I
> don't
> > > see
> > > > a good way to ensure that it gets cleaned up. It can't be
> self-deleting
> > > because
> > > > we have no guarantee that OnURLFetchComplete() (or any other delegate
> > method)
> > > > will ever be called.
> > > >
> > > >
> > > > Did you have an idea here I'm missing?
> > > >
> > > > I can still shift some stuff like the query_task_runner_,
> > query_done_callback_
> > > > and similar things into the delegate object, but if we have to keep
> the
> > > > ownership in PpdProvider, we still end up with this sort of
> pseudo-global
> > > state
> > > > around the fetch.
> > > >
> > > > I'm happy to do that, just not sure it's as worth it without the
> other
> > > benefits.
> > > > Let me know what you think, or if you have another idea.
> > >
> > > So, for now at least, we can continue to have a single
> resolve_fetcher_, and
> > can
> > > continue to check above that resolve_fetcher_ is null. Anywhere we
> reset it
> > > (OnResolveFetchComplete, AbortResolve()), we should also reset the
> associated
> > > delegate.
> > >
> > > We could also just keep the current model but pass the parameters to
> the
> > single
> > > delegate instance each time.
> > >
> > > Either way, I think that wrapping the members associated with the
> current
> > > request in the delegate still makes sense. It at least makes it a bit
> more
> > clear
> > > that those members are tied to the request.
> >
> > Did this. To make it a little more orthogonal, I converted the Delegate
> class
> > to just be a thin wrapper around a callback, otherwise I needed to
> either do
> > separate delegate classes for each kind of url fetch, or do a weird
> pseudo-union
> > of fields in the object; it seems cleaner just to carry the state in the
> passed
> > callback. Let me know what you think.
>
> Oh, forgot to also mention this delta rebases on top of some other changes
> that
> went in recently. Sorry about the extra diffspam.
>
> -J
>
> https://codereview.chromium.org/2476073003/
>
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/12 01:55:49, stevenjb wrote:
> I won't have a chance to re-review this again until Monday, however in
> regard to your questions / concerns:
>
> * We should not do any blocking, you are correct on that account.
> * Usually, the code can be structured such that the primary thread (i.e.
> the thread that invoked the request, usually the UI thread) runs the
> overall state machine, with the io task runner handling blocking requests.
> We try to avoid having a method running on a blocking task runner (e.g. a
> file request) spawn another blocking task (e.g. a URLFetcher request).
>
> In fact, re-scanning the code, it is not clear to me whether calling
> URLFetcher::Create on the blocking task runner is desired or even valid.
>
> So to rephrase my suggestion:
> 1. Rename disk_task_runner_ to blocking_ or io_task_runner since we're
> (maybe?) using it for non disk operations (and either way we really
> shouldn't care what it's for beyond "slow blocking tasks").
> 2. Break up ResolveImpl into (a) a method that does file operations, and
> (b) a method that calls URLFetcher::Create. Even if both need to be done on
> the blocking thread (and we should understand whether or not that is true),
> it will be easier to follow the code if those are separate tasks. Rename
> these methods something like ResloveCachedPpd and ResolvePppUrlFetch (or
> whatever makes sense).
> 3. We really ought to be able to use PostTaskAndReply{WithResponse} instead
> of passing a task runner to the methods. That's pretty much what it is for.
The only real alternative I can see that gets us closer to what you want is,
roughly
PostTaskAndReply() the cached lookup. If the cache lookup succeeds, the
callback to the Resolve() caller is invoked at that point, otherwise it chains
to a URLFetcher call and invokes the Resolve() callback when the fetch finishes.
I *think* this could look reasonably clean, not entirely sure. Let me try it
out and see how it looks.
Yes, that is what I am suggesting. Separate the cache lookup from the URLFetcher request. Cheers, Steven On Mon, Nov 14, 2016 at 10:13 AM, <justincarlson@chromium.org> wrote: > On 2016/11/12 01:55:49, stevenjb wrote: > > I won't have a chance to re-review this again until Monday, however in > > regard to your questions / concerns: > > > > * We should not do any blocking, you are correct on that account. > > * Usually, the code can be structured such that the primary thread (i.e. > > the thread that invoked the request, usually the UI thread) runs the > > overall state machine, with the io task runner handling blocking > requests. > > We try to avoid having a method running on a blocking task runner (e.g. a > > file request) spawn another blocking task (e.g. a URLFetcher request). > > > > In fact, re-scanning the code, it is not clear to me whether calling > > URLFetcher::Create on the blocking task runner is desired or even valid. > > > > So to rephrase my suggestion: > > 1. Rename disk_task_runner_ to blocking_ or io_task_runner since we're > > (maybe?) using it for non disk operations (and either way we really > > shouldn't care what it's for beyond "slow blocking tasks"). > > 2. Break up ResolveImpl into (a) a method that does file operations, and > > (b) a method that calls URLFetcher::Create. Even if both need to be done > on > > the blocking thread (and we should understand whether or not that is > true), > > it will be easier to follow the code if those are separate tasks. Rename > > these methods something like ResloveCachedPpd and ResolvePppUrlFetch (or > > whatever makes sense). > > 3. We really ought to be able to use PostTaskAndReply{WithResponse} > instead > > of passing a task runner to the methods. That's pretty much what it is > for. > > The only real alternative I can see that gets us closer to what you want > is, > roughly > > PostTaskAndReply() the cached lookup. If the cache lookup succeeds, the > callback to the Resolve() caller is invoked at that point, otherwise it > chains > to a URLFetcher call and invokes the Resolve() callback when the fetch > finishes. > > I *think* this could look reasonably clean, not entirely sure. Let me try > it > out and see how it looks. > > > > https://codereview.chromium.org/2476073003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/11/14 18:20:46, stevenjb wrote:
> Yes, that is what I am suggesting. Separate the cache lookup from the
> URLFetcher request.
>
OK, done. It's still got some rough edges that I don't like, mostly because
WeakPtr disallows tasks with return values, so POstTaskAndReplyWithResult can't
be used, but overall I think this is reasonable.
PTAL?
-J
> Cheers,
> Steven
>
>
> On Mon, Nov 14, 2016 at 10:13 AM, <mailto:justincarlson@chromium.org> wrote:
>
> > On 2016/11/12 01:55:49, stevenjb wrote:
> > > I won't have a chance to re-review this again until Monday, however in
> > > regard to your questions / concerns:
> > >
> > > * We should not do any blocking, you are correct on that account.
> > > * Usually, the code can be structured such that the primary thread (i.e.
> > > the thread that invoked the request, usually the UI thread) runs the
> > > overall state machine, with the io task runner handling blocking
> > requests.
> > > We try to avoid having a method running on a blocking task runner (e.g. a
> > > file request) spawn another blocking task (e.g. a URLFetcher request).
> > >
> > > In fact, re-scanning the code, it is not clear to me whether calling
> > > URLFetcher::Create on the blocking task runner is desired or even valid.
> > >
> > > So to rephrase my suggestion:
> > > 1. Rename disk_task_runner_ to blocking_ or io_task_runner since we're
> > > (maybe?) using it for non disk operations (and either way we really
> > > shouldn't care what it's for beyond "slow blocking tasks").
> > > 2. Break up ResolveImpl into (a) a method that does file operations, and
> > > (b) a method that calls URLFetcher::Create. Even if both need to be done
> > on
> > > the blocking thread (and we should understand whether or not that is
> > true),
> > > it will be easier to follow the code if those are separate tasks. Rename
> > > these methods something like ResloveCachedPpd and ResolvePppUrlFetch (or
> > > whatever makes sense).
> > > 3. We really ought to be able to use PostTaskAndReply{WithResponse}
> > instead
> > > of passing a task runner to the methods. That's pretty much what it is
> > for.
> >
> > The only real alternative I can see that gets us closer to what you want
> > is,
> > roughly
> >
> > PostTaskAndReply() the cached lookup. If the cache lookup succeeds, the
> > callback to the Resolve() caller is invoked at that point, otherwise it
> > chains
> > to a URLFetcher call and invokes the Resolve() callback when the fetch
> > finishes.
> >
> > I *think* this could look reasonably clean, not entirely sure. Let me try
> > it
> > out and see how it looks.
> >
> >
> >
> > https://codereview.chromium.org/2476073003/
> >
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
This looks good. The pattern of passing a unique_ptr to a task to propagate to the reply is fine, we use it a fair bit elsewhere. Just some naming suggestions and nits. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:88: CHECK(io_task_runner_->PostTaskAndReply( Don't wrap non test function calls with CHECK, instead store the result and CHECK it separately. (i.e. changing CHECK to DCHECK shouldn't change the logic). https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:89: FROM_HERE, base::Bind(&PpdProviderImpl::CacheFindWrapper, nit: This will be easier to follow if the method is named something like ResolveDoCacheLookup. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:107: FROM_HERE, base::Bind(&PpdProviderImpl::CacheFindAvailableWrapper, QueryAvailableDoCacheLookup https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:109: base::Bind(&PpdProviderImpl::QueryCacheLookupDone, QueryAvailableCacheLookupDone https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:126: // use weak_ptrs to manage lifetime, and so so we need to bind callbacks to s/so so/because/ https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:128: // we have to use a parameter to return state. FWIW, this isn't that uncommon a pattern, you could probably simplify the explanation, e.g.: // Do the cache lookup on the IO thread and set |cache_result| to pass // too ResolveCacheLookupDone. The only reason that "weak_ptr's preclude return values in posted tasks" was an issue is because I suggested PostTaskAndReplyWithResult without considering the lifetime of |cache_| (which is why this needs to be a member function). It's subtle, and something you will remember next time it comes up, but not necessarily relevant to the next maintainer :) https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:132: } blank line https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:147: // was invoked on. Last sentence is unnecessary, we have a CHECK on line 152 that makes that clear. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:198: auto fetcher_delegate(std::move(resolve_fetcher_delegate_)); nit: we don't appear to need fetcher_delegate below, so we can just reset() resolve_fetcher_delegate_. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:282: auto fetcher_delegate(std::move(query_fetcher_delegate_)); Also unused, can use reset(). https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.h:98: // Only one Resolve() call should be outstanding at a time. s/should be/may be/ https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.h:110: // Only one QueryAvailable() call should be outstanding at a time. s/should be/may be/
Thanks for the significant suggestions. I think this is much better off for it. I think I've addressed/responded to everything. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:88: CHECK(io_task_runner_->PostTaskAndReply( On 2016/11/14 23:08:39, stevenjb wrote: > Don't wrap non test function calls with CHECK, instead store the result and > CHECK it separately. (i.e. changing CHECK to DCHECK shouldn't change the logic). > Done, but for my own edification, is this just your personal preference, or something style-guide related? https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:89: FROM_HERE, base::Bind(&PpdProviderImpl::CacheFindWrapper, On 2016/11/14 23:08:39, stevenjb wrote: > nit: This will be easier to follow if the method is named something like > ResolveDoCacheLookup. > Done. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:107: FROM_HERE, base::Bind(&PpdProviderImpl::CacheFindAvailableWrapper, On 2016/11/14 23:08:39, stevenjb wrote: > QueryAvailableDoCacheLookup Done. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:109: base::Bind(&PpdProviderImpl::QueryCacheLookupDone, On 2016/11/14 23:08:39, stevenjb wrote: > QueryAvailableCacheLookupDone Done. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:126: // use weak_ptrs to manage lifetime, and so so we need to bind callbacks to On 2016/11/14 23:08:39, stevenjb wrote: > s/so so/because/ Done. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:128: // we have to use a parameter to return state. On 2016/11/14 23:08:39, stevenjb wrote: > FWIW, this isn't that uncommon a pattern, you could probably simplify the > explanation, e.g.: > > // Do the cache lookup on the IO thread and set |cache_result| to pass > // too ResolveCacheLookupDone. > > The only reason that "weak_ptr's preclude return values in posted tasks" was an > issue is because I suggested PostTaskAndReplyWithResult without considering the > lifetime of |cache_| (which is why this needs to be a member function). It's > subtle, and something you will remember next time it comes up, but not > necessarily relevant to the next maintainer :) From the perspective of someone new to this code base, I'm actually pretty convinced that the Chromium code base is rather heavy on undocumented convention and subtleties that surprise a new person to the code. So I'd prefer to err on the side of too much information about why this is, rather than too little. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:132: } On 2016/11/14 23:08:39, stevenjb wrote: > blank line Done. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:147: // was invoked on. On 2016/11/14 23:08:39, stevenjb wrote: > Last sentence is unnecessary, we have a CHECK on line 152 that makes that clear. Sort of, the CHECK below just guarantees sequence, not thread affinity, but generally again I'd really like to err on the side of too much explanation than too little here. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:198: auto fetcher_delegate(std::move(resolve_fetcher_delegate_)); On 2016/11/14 23:08:39, stevenjb wrote: > nit: we don't appear to need fetcher_delegate below, so we can just reset() > resolve_fetcher_delegate_. The delegate holds the callback *currently being invoked*, which I think means it also implicitly holds the memory for the (const reference) parameters being passed into this function, doesn't it? (I may have this wrong, the subtleties of Bind() and related wizardry are deep and mysterious to me. :)) https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:282: auto fetcher_delegate(std::move(query_fetcher_delegate_)); On 2016/11/14 23:08:39, stevenjb wrote: > Also unused, can use reset(). See above https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.h (right): https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.h:110: // Only one QueryAvailable() call should be outstanding at a time. On 2016/11/14 23:08:39, stevenjb wrote: > s/should be/may be/ Done.
lgtm https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... File chromeos/printing/ppd_provider.cc (right): https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:88: CHECK(io_task_runner_->PostTaskAndReply( On 2016/11/14 23:27:41, Carlson wrote: > On 2016/11/14 23:08:39, stevenjb wrote: > > Don't wrap non test function calls with CHECK, instead store the result and > > CHECK it separately. (i.e. changing CHECK to DCHECK shouldn't change the > logic). > > > > Done, but for my own edification, is this just your personal preference, or > something style-guide related? It's not explicitly in the guidelines, however this actually ought to be a DCHECK, see the description: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... So, because CHECK should be used sparingly, the pattern of CHECK(DoSomething()) is rare. Also, FWIW, I rarely see the result of PostTask checked; if PostTask is failing, it will almost certainly be caught at a higher level. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:128: // we have to use a parameter to return state. On 2016/11/14 23:27:41, Carlson wrote: > On 2016/11/14 23:08:39, stevenjb wrote: > > FWIW, this isn't that uncommon a pattern, you could probably simplify the > > explanation, e.g.: > > > > // Do the cache lookup on the IO thread and set |cache_result| to pass > > // too ResolveCacheLookupDone. > > > > The only reason that "weak_ptr's preclude return values in posted tasks" was > an > > issue is because I suggested PostTaskAndReplyWithResult without considering > the > > lifetime of |cache_| (which is why this needs to be a member function). It's > > subtle, and something you will remember next time it comes up, but not > > necessarily relevant to the next maintainer :) > > From the perspective of someone new to this code base, I'm actually pretty > convinced that the Chromium code base is rather heavy on undocumented convention > and subtleties that surprise a new person to the code. So I'd prefer to err on > the side of too much information about why this is, rather than too little. Acknowledged, but over-documentation is it's own problem. Anyway, it's fine. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:147: // was invoked on. On 2016/11/14 23:27:41, Carlson wrote: > On 2016/11/14 23:08:39, stevenjb wrote: > > Last sentence is unnecessary, we have a CHECK on line 152 that makes that > clear. > > Sort of, the CHECK below just guarantees sequence, not thread affinity, but > generally again I'd really like to err on the side of too much explanation than > too little here. Sure, but in practice this will only get called from the UI thread or a SingleThreadTaskRunner for the UI thread. Again, we aim for sparse, focused comments that are not clear from inspection. This is called from io_task_runner_->PostTaskAndReply, which is well documented. https://codereview.chromium.org/2476073003/diff/100001/chromeos/printing/ppd_... chromeos/printing/ppd_provider.cc:198: auto fetcher_delegate(std::move(resolve_fetcher_delegate_)); On 2016/11/14 23:27:41, Carlson wrote: > On 2016/11/14 23:08:39, stevenjb wrote: > > nit: we don't appear to need fetcher_delegate below, so we can just reset() > > resolve_fetcher_delegate_. > > The delegate holds the callback *currently being invoked*, which I think means > it also implicitly holds the memory for the (const reference) parameters being > passed into this function, doesn't it? > > (I may have this wrong, the subtleties of Bind() and related wizardry are deep > and mysterious to me. :)) Once the function object is invoked, any bound parameters will be on the stack. Still, that's potentially confusing. I'm sure there's a better model for this, but it's fine as-is.
The CQ bit was checked by justincarlson@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 checked by justincarlson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skau@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2476073003/#ps160001 (title: "Change CHECK to DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Update PpdProvider threading model. We are crashing the browser right now because PpdProvider does disk operations (via PpdCache), and those are invoked on the UI thread. Fix this by making PpdProvider take an explicit task runner on which to do disk operations and pull them off the UI thread. BUG=662626 ========== to ========== Update PpdProvider threading model. We are crashing the browser right now because PpdProvider does disk operations (via PpdCache), and those are invoked on the UI thread. Fix this by making PpdProvider take an explicit task runner on which to do disk operations and pull them off the UI thread. BUG=662626 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Update PpdProvider threading model. We are crashing the browser right now because PpdProvider does disk operations (via PpdCache), and those are invoked on the UI thread. Fix this by making PpdProvider take an explicit task runner on which to do disk operations and pull them off the UI thread. BUG=662626 ========== to ========== Update PpdProvider threading model. We are crashing the browser right now because PpdProvider does disk operations (via PpdCache), and those are invoked on the UI thread. Fix this by making PpdProvider take an explicit task runner on which to do disk operations and pull them off the UI thread. BUG=662626 Committed: https://crrev.com/c31c58ba48632b1713cde7c0e83483595907fb20 Cr-Commit-Position: refs/heads/master@{#432062} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c31c58ba48632b1713cde7c0e83483595907fb20 Cr-Commit-Position: refs/heads/master@{#432062} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
