Create a dedicated cache thread and use it to create the...

Create a dedicated cache thread and use it to create theHttp cache.BUG=26730TEST=noneCommitted: http://src.chromium.org/viewvc/chrome?view=rev&revision=47564

[jam]

http://codereview.chromium.org/1989014/diff/17001/18007
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/1989014/diff/17001/18007#newcode228
chrome/browser/net/chrome_url_request_context.cc:228: MessageLoop*
cache_thread_;
why cache it in these places instead of using ChromeThread to get it when
needed?

[rvargas]

http://codereview.chromium.org/1989014/diff/17001/18007
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/1989014/diff/17001/18007#newcode228
chrome/browser/net/chrome_url_request_context.cc:228: MessageLoop*
cache_thread_;
On 2010/05/13 18:18:31, John Abd-El-Malek wrote:
> why cache it in these places instead of using ChromeThread to get it when
> needed?

The constructor of this factory runs on the UI thread, but Create() runs on the
IO thread (when we send the cached loop to the Http Cache constructor).

[jam]

http://codereview.chromium.org/1989014/diff/17001/18007
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/1989014/diff/17001/18007#newcode228
chrome/browser/net/chrome_url_request_context.cc:228: MessageLoop*
cache_thread_;
On 2010/05/13 19:36:54, rvargas wrote:
> On 2010/05/13 18:18:31, John Abd-El-Malek wrote:
> > why cache it in these places instead of using ChromeThread to get it when
> > needed?
> 
> The constructor of this factory runs on the UI thread, but Create() runs on
the
> IO thread (when we send the cached loop to the Http Cache constructor).
> 

I see, so it's because you want the raw MessageLoop pointer to pass to code
outside of browser.  This causes headaches at shutdown because of differences in
thread lifetimes.  Sanjeev just added a solution to this last week with the
MessageLoopProxy class.  It's a safe way of passing MessageLoop's around, and in
the Create() function below, you can call
ChromeThread::GetMessageLoopProxyForThread(ChromeThread::CACHE) and the code in
net can use a MessageLoopProxy.

[rvargas]

On 2010/05/13 19:50:44, John Abd-El-Malek wrote:
> http://codereview.chromium.org/1989014/diff/17001/18007
> File chrome/browser/net/chrome_url_request_context.cc (right):
> 
> http://codereview.chromium.org/1989014/diff/17001/18007#newcode228
> chrome/browser/net/chrome_url_request_context.cc:228: MessageLoop*
> cache_thread_;
> On 2010/05/13 19:36:54, rvargas wrote:
> > On 2010/05/13 18:18:31, John Abd-El-Malek wrote:
> > > why cache it in these places instead of using ChromeThread to get it when
> > > needed?
> > 
> > The constructor of this factory runs on the UI thread, but Create() runs on
> the
> > IO thread (when we send the cached loop to the Http Cache constructor).
> > 
> 
> I see, so it's because you want the raw MessageLoop pointer to pass to code
> outside of browser.  This causes headaches at shutdown because of differences
in
> thread lifetimes.  Sanjeev just added a solution to this last week with the
> MessageLoopProxy class.  It's a safe way of passing MessageLoop's around, and
in
> the Create() function below, you can call
> ChromeThread::GetMessageLoopProxyForThread(ChromeThread::CACHE) and the code
in
> net can use a MessageLoopProxy.

Sorry for the delay.

Yeah... I had this code sitting in my PC for over a month :(

So, is the idea to replace all uses of MessageLoop pointers with a proxy? (in
that case, a few suggestions made on Sanjeev's code review would have been worth
exploring). I know we have spent a lot of effort trying to simplify shutdown
(and making it more reliable), but I don't see how having a proxy improves
reliability in this case... the thread lifetime is controlled by the browser in
the proper order, and the only user of the thread (the disk cache) is  aware of
all things that should happen when the destructor is called: today there is
synchronization to make sure everything is done before returning, and the same
thing holds true for work being done on the new thread.

The only trouble would be if the cache thread destruction starts before the io
thread destruction finishes, but I thought that case was prevented by the
joining of the io thread before moving on with the next thread destruction.

I guess I can change all the other code to use a proxy if you still want me to.

[jam]

On Mon, May 17, 2010 at 3:06 PM, <rvargas@chromium.org> wrote:

> On 2010/05/13 19:50:44, John Abd-El-Malek wrote:
>
>> http://codereview.chromium.org/1989014/diff/17001/18007
>> File chrome/browser/net/chrome_url_request_context.cc (right):
>>
>
>  http://codereview.chromium.org/1989014/diff/17001/18007#newcode228
>> chrome/browser/net/chrome_url_request_context.cc:228: MessageLoop*
>> cache_thread_;
>> On 2010/05/13 19:36:54, rvargas wrote:
>> > On 2010/05/13 18:18:31, John Abd-El-Malek wrote:
>> > > why cache it in these places instead of using ChromeThread to get it
>> when
>> > > needed?
>> >
>> > The constructor of this factory runs on the UI thread, but Create() runs
>> on
>> the
>> > IO thread (when we send the cached loop to the Http Cache constructor).
>> >
>>
>
>  I see, so it's because you want the raw MessageLoop pointer to pass to
>> code
>> outside of browser.  This causes headaches at shutdown because of
>> differences
>>
> in
>
>> thread lifetimes.  Sanjeev just added a solution to this last week with
>> the
>> MessageLoopProxy class.  It's a safe way of passing MessageLoop's around,
>> and
>>
> in
>
>> the Create() function below, you can call
>> ChromeThread::GetMessageLoopProxyForThread(ChromeThread::CACHE) and the
>> code
>>
> in
>
>> net can use a MessageLoopProxy.
>>
>
> Sorry for the delay.
>
> Yeah... I had this code sitting in my PC for over a month :(
>
> So, is the idea to replace all uses of MessageLoop pointers with a proxy?


That would be the goal, since right now we pass MessageLoop pointers all
over the code and it's hard to know which cases are safe and which aren't.


> (in
> that case, a few suggestions made on Sanjeev's code review would have been
> worth
> exploring).


I think there's always time for that :)


> I know we have spent a lot of effort trying to simplify shutdown
> (and making it more reliable), but I don't see how having a proxy improves
> reliability in this case...


The goal is to make all usage safe, without having to control the threads'
lifetime so closely.  I'm sure you've seen how annoying/complicated it is to
ensure that the thread's lifetimes are coordinated with whoever could be
holding a pointer to their ML.


> the thread lifetime is controlled by the browser in
> the proper order, and the only user of the thread (the disk cache) is
>  aware of
> all things that should happen when the destructor is called: today there is
> synchronization to make sure everything is done before returning, and the
> same
> thing holds true for work being done on the new thread.
>
> The only trouble would be if the cache thread destruction starts before the
> io
> thread destruction finishes, but I thought that case was prevented by the
> joining of the io thread before moving on with the next thread destruction.
>
> I guess I can change all the other code to use a proxy if you still want me
> to.


If it's a lot of work, then don't worry about it.  But if it's something
that could be done quickly, I think it'd be great.

>
>
> http://codereview.chromium.org/1989014/show
>

[rvargas]

Please take another look.

Thanks.

[jam]

http://codereview.chromium.org/1989014/diff/33001/34011
File net/http/http_cache.h (right):

http://codereview.chromium.org/1989014/diff/33001/34011#newcode111
net/http/http_cache.h:111: base::MessageLoopProxy* thread_;
I think this needs to be an actual MLP object instead of a pointer, otherwise
you lose the benefits of this wrapper.

[rvargas]

thanks.

http://codereview.chromium.org/1989014/diff/33001/34011
File net/http/http_cache.h (right):

http://codereview.chromium.org/1989014/diff/33001/34011#newcode111
net/http/http_cache.h:111: base::MessageLoopProxy* thread_;
On 2010/05/18 17:27:23, John Abd-El-Malek wrote:
> I think this needs to be an actual MLP object instead of a pointer, otherwise
> you lose the benefits of this wrapper.

Done.

[jam]

lgtm