Remove base::WorkerPool use from FileStream tests.

Remove base::WorkerPool use from FileStream tests.

Use the main message loop for async operations in FileStream tests. Previously they were scheduled in base::WorkerPool.

Removes dependency on deprecated code and plugs memory leaks.

BUG=248513
R=rsleevi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221926

[earthdok, 2013-09-05 11:03:06]

Please take a look.

Flushing the main message loop before and after stopping the thread wasn't
enough to get rid of all the leaks, so I had to pump the thread one extra time.
There's probably a more idiomatic way of doing that, but I couldn't find one.

[Ryan Sleevi, 2013-09-06 20:46:38]

https://codereview.chromium.org/23872004/diff/1/net/base/file_stream_unittest.cc
File net/base/file_stream_unittest.cc (right):

https://codereview.chromium.org/23872004/diff/1/net/base/file_stream_unittest...
net/base/file_stream_unittest.cc:75: base::Thread file_thread_;
The only requirement for this interface is that there is a TaskRunner.

Why do you not use MessageLoop::current()? What of the design prevents this?

We should definitely be avoiding creating threads for tests if at all possible.

[earthdok, 2013-09-06 22:33:40]

On 2013/09/06 20:46:38, Ryan Sleevi wrote:
>
https://codereview.chromium.org/23872004/diff/1/net/base/file_stream_unittest.cc
> File net/base/file_stream_unittest.cc (right):
> 
>
https://codereview.chromium.org/23872004/diff/1/net/base/file_stream_unittest...
> net/base/file_stream_unittest.cc:75: base::Thread file_thread_;
> The only requirement for this interface is that there is a TaskRunner.
> 
> Why do you not use MessageLoop::current()? What of the design prevents this?

I guess I just opted for the less intrusive change (since using the message loop
would require us to explicitly pump it after each async call).  I agree with you
though - will fix.

[earthdok, 2013-09-06 22:52:00]

On 2013/09/06 22:33:40, earthdok wrote:
> On 2013/09/06 20:46:38, Ryan Sleevi wrote:
> >
>
https://codereview.chromium.org/23872004/diff/1/net/base/file_stream_unittest.cc
> > File net/base/file_stream_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/23872004/diff/1/net/base/file_stream_unittest...
> > net/base/file_stream_unittest.cc:75: base::Thread file_thread_;
> > The only requirement for this interface is that there is a TaskRunner.
> > 
> > Why do you not use MessageLoop::current()? What of the design prevents this?
> 
> I guess I just opted for the less intrusive change (since using the message
loop
> would require us to explicitly pump it after each async call).  I agree with
you
> though - will fix.

Fixed. It turns out that the callbacks do all the pumping for us, so everything
works out splendidly.

[Ryan Sleevi, 2013-09-06 23:27:27]

https://codereview.chromium.org/23872004/diff/7001/net/base/file_stream_unitt...
File net/base/file_stream_unittest.cc (right):

https://codereview.chromium.org/23872004/diff/7001/net/base/file_stream_unitt...
net/base/file_stream_unittest.cc:51: base::RunLoop().RunUntilIdle();
I'm not really thrilled with adding this to test shutdown.

In general, we should try to have our tests not leave tasks hanging. Is this
something you can go through and figure out what Tests need to pump tasks, and
pump them accordingly?

[I also have no clue why this test is using SetUp/TearDown, rather than just
doing this in the ctor/dtor as good GTests should do]

https://codereview.chromium.org/23872004/diff/7001/net/base/file_stream_unitt...
net/base/file_stream_unittest.cc:58: return
base::MessageLoop::current()->message_loop_proxy();
Just drop this method and use base::MessageLoopProxy::current(), rather than
indirecting through the MessageLoop. (eg: updating 70 to take the
MLP::Current())

[earthdok, 2013-09-07 00:20:03]

https://codereview.chromium.org/23872004/diff/7001/net/base/file_stream_unitt...
File net/base/file_stream_unittest.cc (right):

https://codereview.chromium.org/23872004/diff/7001/net/base/file_stream_unitt...
net/base/file_stream_unittest.cc:51: base::RunLoop().RunUntilIdle();
On 2013/09/06 23:27:28, Ryan Sleevi wrote:
> I'm not really thrilled with adding this to test shutdown.
> 
> In general, we should try to have our tests not leave tasks hanging. Is this
> something you can go through and figure out what Tests need to pump tasks, and
> pump them accordingly?
The tasks are posted from async FileStreams on destruction. I have no problem
with adding a nested scope + pumping code to each async test if that's what you
prefer, but to me it's just unnecessary code duplication. This idiom (pumping
the MessageLoop in TearDown) is quite widespread anyway.

[earthdok, 2013-09-07 00:23:09]

On 2013/09/07 00:20:03, earthdok wrote:
>
https://codereview.chromium.org/23872004/diff/7001/net/base/file_stream_unitt...
> File net/base/file_stream_unittest.cc (right):
> 
>
https://codereview.chromium.org/23872004/diff/7001/net/base/file_stream_unitt...
> net/base/file_stream_unittest.cc:51: base::RunLoop().RunUntilIdle();
> On 2013/09/06 23:27:28, Ryan Sleevi wrote:
> > I'm not really thrilled with adding this to test shutdown.
> > 
> > In general, we should try to have our tests not leave tasks hanging. Is this
> > something you can go through and figure out what Tests need to pump tasks,
and
> > pump them accordingly?
> The tasks are posted from async FileStreams on destruction. I have no problem
> with adding a nested scope + pumping code to each async test if that's what
you
> prefer, but to me it's just unnecessary code duplication. This idiom (pumping
> the MessageLoop in TearDown) is quite widespread anyway.

Would it make you happy if I just put a comment there e.g. "Pump any pending
asynchronous tasks"?

[Ryan Sleevi, 2013-09-07 00:31:29]

On 2013/09/07 00:23:09, earthdok wrote:
> On 2013/09/07 00:20:03, earthdok wrote:
> >
>
https://codereview.chromium.org/23872004/diff/7001/net/base/file_stream_unitt...
> > File net/base/file_stream_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/23872004/diff/7001/net/base/file_stream_unitt...
> > net/base/file_stream_unittest.cc:51: base::RunLoop().RunUntilIdle();
> > On 2013/09/06 23:27:28, Ryan Sleevi wrote:
> > > I'm not really thrilled with adding this to test shutdown.
> > > 
> > > In general, we should try to have our tests not leave tasks hanging. Is
this
> > > something you can go through and figure out what Tests need to pump tasks,
> and
> > > pump them accordingly?
> > The tasks are posted from async FileStreams on destruction. I have no
problem
> > with adding a nested scope + pumping code to each async test if that's what
> you
> > prefer, but to me it's just unnecessary code duplication. This idiom
(pumping
> > the MessageLoop in TearDown) is quite widespread anyway.
> 
> Would it make you happy if I just put a comment there e.g. "Pump any pending
> asynchronous tasks"?

Because the SetUp code is not responsible for the part of FileStream
initialization, I'm not thrilled with having the TearDown code responsible for
the cleanup.

That said, I think a specific comment here is helpful.

// FileStreams must be asynchronously closed on the file task runner before they
can be deleted. Pump the RunLoop to avoid leaks.

Or something equivalent indicating exactly what behaviour you're pumping the
RunLoop for, since, in general, we don't want Tests to end while they still have
tasks pending.

[earthdok, 2013-09-07 00:57:32]

all addressed

[Ryan Sleevi, 2013-09-07 01:54:45]

lgtm

[commit-bot: I haz the power, 2013-09-07 01:54:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/earthdok@chromium.org/23872004/18001

[Ryan Sleevi, 2013-09-07 01:55:07]

LGTM. 

Please update description to reflect the status of the CL (no longer using a
dedicated thread)

[commit-bot: I haz the power, 2013-09-07 10:45:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/earthdok@chromium.org/23872004/18001

[commit-bot: I haz the power, 2013-09-07 16:01:54]

Change committed as 221926