Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
617393005:
Make URLRequestContextAdapter initialization asynchronous (Closed)
DescriptionMake URLRequestContextAdapter initialization asynchronous
Currently we place a 2-second on URLRequestContextAdapter initialization, so
that context can be initialized when embedders make requests. This CL changes
so that tasks that depend on context being initialized will be run after the
context is initialized properly, and URLRequestContextAdapter initialization
won't block.
BUG=
Committed: https://crrev.com/be3c72f84d1b9421948cc1b1a813ee562cd74666
Cr-Commit-Position: refs/heads/master@{#299145}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed Comments #Patch Set 3 : Addressed comments #
Total comments: 1
Patch Set 4 : Removed mStarted #
Total comments: 8
Patch Set 5 : Addressed comments #
Total comments: 4
Patch Set 6 : Addressed comments #
Total comments: 12
Patch Set 7 : Addressed comments #
Total comments: 4
Patch Set 8 : Addressed Misha's comments #
Messages
Total messages: 24 (3 generated)
xunjieli@chromium.org changed reviewers: + mef@chromium.org, mmenke@chromium.org
This is the CL that implements the change we talked about earlier. PTAL. Thanks!
My main concern is testing this - it's complicated enough that we should have tests that force the network stack to be initialized late in the game and a couple commands on the stack, to make sure this works. Another concern: We'll want to implement wrappers for blocking APIs on top of this. If there's a wrapped blocking network call on the main thread (Which is a horrible thing to do, of course), we're in trouble. Even if it's done after we're fully initialized, we still won't be able to get notifications. We can either choose to live with this, since people shouldn't be doing blocking network IO on the main thread, or use a workaround. The only way I can think of to handle this is create our own network Java thread to receive notifications on. Problem with that is that Java threads are fairly heavy weight, I believe. So it may actually be best to just use the main thread. https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.cc:85: is_last_chunk)); Need to do the same here (And for basically everything that posts a task to the network thread). https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.cc:161: base::Bind(&URLRequestAdapter::OnCancelRequest, base::Unretained(this))); Need to do the same here. A bit of a pain, but we should spend some time and figure out how to test the case of delayed initialization. https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_context_adapter.h:28: typedef struct base::Callback<void()> RunAfterContextInitTask; nit: Know I was the one who added the "struct", but looks a bit more common not to use it. https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_context_adapter.h:85: std::queue<RunAfterContextInitTask> tasks_; Should include <queue> https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_context_adapter.h:85: std::queue<RunAfterContextInitTask> tasks_; Maybe tasks_waiting_for_context_?
Oh, and I should add - we should make sure we both have tests where we try and start (And cancel) requests before the network stack is set up, and where we try and start them (And cancel them) after the network stack is set up.
Thanks Matt for bringing up the cases to think about! I have uploaded a new patch to address the comments. Will do the testing in my other CL, as discussed. PTAL. https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.cc:85: is_last_chunk)); On 2014/10/02 19:51:18, mmenke wrote: > Need to do the same here (And for basically everything that posts a task to the > network thread). Done. https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_adapter.cc:161: base::Bind(&URLRequestAdapter::OnCancelRequest, base::Unretained(this))); On 2014/10/02 19:51:17, mmenke wrote: > Need to do the same here. A bit of a pain, but we should spend some time and > figure out how to test the case of delayed initialization. Done. https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_context_adapter.h:28: typedef struct base::Callback<void()> RunAfterContextInitTask; On 2014/10/02 19:51:18, mmenke wrote: > nit: Know I was the one who added the "struct", but looks a bit more common not > to use it. Done. https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_context_adapter.h:85: std::queue<RunAfterContextInitTask> tasks_; On 2014/10/02 19:51:18, mmenke wrote: > Should include <queue> Done. https://codereview.chromium.org/617393005/diff/1/components/cronet/android/ur... components/cronet/android/url_request_context_adapter.h:85: std::queue<RunAfterContextInitTask> tasks_; On 2014/10/02 19:51:18, mmenke wrote: > Maybe tasks_waiting_for_context_? Done.
https://codereview.chromium.org/617393005/diff/40001/components/cronet/androi... File components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java (right): https://codereview.chromium.org/617393005/diff/40001/components/cronet/androi... components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:91: mStarted.open(); I should probably get rid of mStarted completely, since there is no blocking call. Will do it now.
https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:173: FROM_HERE, base::Bind(&URLRequestAdapter::OnDestroyRequest, this)); Sorry, this should also use RunTask, as it's theoretically possible to even destroy an adapter before the context is initialized. https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... components/cronet/android/url_request_context_adapter.cc:198: // This method can be called on any thread. nit: Function level comments should go with the declaration, not the definition. https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... components/cronet/android/url_request_context_adapter.cc:243: void URLRequestContextAdapter::StartNetLogToFile(const std::string& file_name) { This also has to use RunTask now (Same with StopNetLog). https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... components/cronet/android/url_request_context_adapter.h:63: void RunTask(const RunAfterContextInitTask& callback); Maybe RunTaskAfterContextInit? It's a bit unwieldy, but makes it clearer what's going on at callsites, where the callback's type is less obvious. Same for the next method.
Patchset #5 (id:80001) has been deleted
Thanks for the review! I have uploaded a new patch to address the comments. PTAL. https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... components/cronet/android/url_request_adapter.cc:173: FROM_HERE, base::Bind(&URLRequestAdapter::OnDestroyRequest, this)); On 2014/10/02 22:11:52, mmenke wrote: > Sorry, this should also use RunTask, as it's theoretically possible to even > destroy an adapter before the context is initialized. Done. https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... components/cronet/android/url_request_context_adapter.cc:198: // This method can be called on any thread. On 2014/10/02 22:11:52, mmenke wrote: > nit: Function level comments should go with the declaration, not the > definition. Done. https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... components/cronet/android/url_request_context_adapter.cc:243: void URLRequestContextAdapter::StartNetLogToFile(const std::string& file_name) { On 2014/10/02 22:11:52, mmenke wrote: > This also has to use RunTask now (Same with StopNetLog). Done. https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/617393005/diff/60001/components/cronet/androi... components/cronet/android/url_request_context_adapter.h:63: void RunTask(const RunAfterContextInitTask& callback); On 2014/10/02 22:11:52, mmenke wrote: > Maybe RunTaskAfterContextInit? It's a bit unwieldy, but makes it clearer what's > going on at callsites, where the callback's type is less obvious. Same for the > next method. Done.
Just suggesting some DCHECKs out of paranoia, and then I'm happy with this. https://codereview.chromium.org/617393005/diff/100001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/617393005/diff/100001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:178: void URLRequestAdapter::OnDestroyRequest(URLRequestAdapter* self) { Suggest adding a: DCHECK(context_->GetNetworkTaskRunner()->BelongsToCurrentThread()); To every function that should run on the network thread: OnDestroyRequest, OnResponseStarted, Read, etc - basically every function below this point, I believe. Will also need to include the SingleThreadedTaskRunner header in this file. https://codereview.chromium.org/617393005/diff/100001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/617393005/diff/100001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:231: net::URLRequestContext* URLRequestContextAdapter::GetURLRequestContext() { DCHECK(context_->GetNetworkTaskRunner()->BelongsToCurrentThread()); Also for other methods that must be called on the network thread.
I have uploaded a new patch to address the comments. PTAL. https://codereview.chromium.org/617393005/diff/100001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/617393005/diff/100001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:178: void URLRequestAdapter::OnDestroyRequest(URLRequestAdapter* self) { On 2014/10/03 19:36:14, mmenke wrote: > Suggest adding a: > > DCHECK(context_->GetNetworkTaskRunner()->BelongsToCurrentThread()); > > To every function that should run on the network thread: > OnDestroyRequest, OnResponseStarted, Read, etc - basically every function below > this point, I believe. > > Will also need to include the SingleThreadedTaskRunner header in this file. Done. https://codereview.chromium.org/617393005/diff/100001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/617393005/diff/100001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:231: net::URLRequestContext* URLRequestContextAdapter::GetURLRequestContext() { On 2014/10/03 19:36:14, mmenke wrote: > DCHECK(context_->GetNetworkTaskRunner()->BelongsToCurrentThread()); > > Also for other methods that must be called on the network thread. Done. https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... File components/cronet/android/url_request_adapter.h (right): https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_adapter.h:119: bool OnNetworkThread() const; I made this public, coz URLRequestAdapter::OnDestroyRequest is static. URLRequestAdapter::OnDestroyRequest only has access to the adapter, but not the context. Not sure if there's a better way.
LGTM, just nits. https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:43: URLRequestAdapter::~URLRequestAdapter() { Could have a DCHECK(OnNetworkThread()); here, too. A bit redundant with OnDestroyRequest, but makes it clearer what's going on, so think makes sense in both places. https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... File components/cronet/android/url_request_adapter.h (right): https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_adapter.h:119: bool OnNetworkThread() const; On 2014/10/03 20:09:55, xunjieli wrote: > I made this public, coz URLRequestAdapter::OnDestroyRequest is static. > URLRequestAdapter::OnDestroyRequest only has access to the adapter, but not the > context. > > Not sure if there's a better way. This is fine. https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_adapter.h:119: bool OnNetworkThread() const; nit: Add linebreak before method. https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:218: URLRequestContextAdapter::~URLRequestContextAdapter() { DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread()); https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:99: // Helper function to start netlog. This should only be run after context is nit: "...to start writing NetLog data to file." https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:102: // Helper function to stop netlog. This should only be run after context is nit: "...to stop writing NetLog data file file."
Thanks for the review! I have also put a DCHECK(OnNetworkThread()) in URLRequestAdapter::OnAppendChunk, URLRequestAdapter::OnInitiateConnection, URLRequestAdapter::OnCancelRequest. I will wait for Misha to get a chance to have a look at the CL. https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:43: URLRequestAdapter::~URLRequestAdapter() { On 2014/10/03 20:26:58, mmenke wrote: > Could have a DCHECK(OnNetworkThread()); here, too. A bit redundant with > OnDestroyRequest, but makes it clearer what's going on, so think makes sense in > both places. Done. https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... File components/cronet/android/url_request_adapter.h (right): https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_adapter.h:119: bool OnNetworkThread() const; On 2014/10/03 20:26:58, mmenke wrote: > nit: Add linebreak before method. Done. https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:218: URLRequestContextAdapter::~URLRequestContextAdapter() { On 2014/10/03 20:26:58, mmenke wrote: > DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread()); Done. https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.h (right): https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:99: // Helper function to start netlog. This should only be run after context is On 2014/10/03 20:26:58, mmenke wrote: > nit: "...to start writing NetLog data to file." Done. https://codereview.chromium.org/617393005/diff/120001/components/cronet/andro... components/cronet/android/url_request_context_adapter.h:102: // Helper function to stop netlog. This should only be run after context is On 2014/10/03 20:26:58, mmenke wrote: > nit: "...to stop writing NetLog data file file." Done.
Pinging for reviews. Thanks!
On 2014/10/10 13:19:10, xunjieli wrote: > Pinging for reviews. Thanks! Not sure if you want a re-review from me, or are just asking Misha for a review - generally, when someone signs off on a review with nits, no re-review is needed, unless you made other big changes. It's best to be specific when pinging so people are clear on who's being pinged. Anyhow, this still LGTM.
Ok. Thanks Matt! Misha, could you take a look at this CL? thanks! On Fri, Oct 10, 2014 at 10:25 AM, <mmenke@chromium.org> wrote: > On 2014/10/10 13:19:10, xunjieli wrote: > >> Pinging for reviews. Thanks! >> > > Not sure if you want a re-review from me, or are just asking Misha for a > review > - generally, when someone signs off on a review with nits, no re-review is > needed, unless you made other big changes. It's best to be specific when > pinging so people are clear on who's being pinged. > > Anyhow, this still LGTM. > > https://codereview.chromium.org/617393005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/10/10 14:45:33, xunjieli wrote: > Ok. Thanks Matt! Misha, could you take a look at this CL? thanks! > > On Fri, Oct 10, 2014 at 10:25 AM, <mailto:mmenke@chromium.org> wrote: > > > On 2014/10/10 13:19:10, xunjieli wrote: > > > >> Pinging for reviews. Thanks! > >> > > > > Not sure if you want a re-review from me, or are just asking Misha for a > > review > > - generally, when someone signs off on a review with nits, no re-review is > > needed, unless you made other big changes. It's best to be specific when > > pinging so people are clear on who's being pinged. > > > > Anyhow, this still LGTM. > > > > https://codereview.chromium.org/617393005/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Sure, will do now, sorry for the delay.
Looks pretty good with couple of questions / suggestions. Also, what about tests? https://codereview.chromium.org/617393005/diff/140001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/617393005/diff/140001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:162: context_->RunTaskAfterContextInit( Does it make sense to make |RunTaskAfterContextInit| name also reflect the fact that tasks is going to run on network thread? Maybe call it |PostTaskToNetworkThread|? This would match |OnNetworkThread| check. https://codereview.chromium.org/617393005/diff/140001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/617393005/diff/140001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:201: FROM_HERE, I'm a bit concerned that all posted tasks will now come FROM_HERE. Would it make sense to pass tracked_objects::Location as a parameter?
The tests are in a follow-up CL (https://codereview.chromium.org/624443003/), since this CL does not introduce any new cases will fail. Current tests are passing. PTAL. https://codereview.chromium.org/617393005/diff/140001/components/cronet/andro... File components/cronet/android/url_request_adapter.cc (right): https://codereview.chromium.org/617393005/diff/140001/components/cronet/andro... components/cronet/android/url_request_adapter.cc:162: context_->RunTaskAfterContextInit( On 2014/10/10 15:06:27, mef wrote: > Does it make sense to make |RunTaskAfterContextInit| name also reflect the fact > that tasks is going to run on network thread? > > Maybe call it |PostTaskToNetworkThread|? This would match |OnNetworkThread| > check. Done. The "run after context init" part is sort of lost in the new function name, but I think |PostTaskToNetworkThread| does sound cleaner. https://codereview.chromium.org/617393005/diff/140001/components/cronet/andro... File components/cronet/android/url_request_context_adapter.cc (right): https://codereview.chromium.org/617393005/diff/140001/components/cronet/andro... components/cronet/android/url_request_context_adapter.cc:201: FROM_HERE, On 2014/10/10 15:06:27, mef wrote: > I'm a bit concerned that all posted tasks will now come FROM_HERE. Would it make > sense to pass tracked_objects::Location as a parameter? Done. Good idea!
lgtm
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/617393005/210001
Message was sent while issue was closed.
Committed patchset #8 (id:210001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/be3c72f84d1b9421948cc1b1a813ee562cd74666 Cr-Commit-Position: refs/heads/master@{#299145} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
