Add ThreadRestrictions to FileStream.

Add ThreadRestrictions to FileStream.

BUG=60211
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77514

[willchan no longer on Chromium, 2011-03-09 04:26:48]

I want to add it to more places in there, such as Open() and Close(), but I am
pretty sure they won't work yet.

[Evan Martin, 2011-03-09 19:14:26]

LGTM

Maybe add BUG=60211

http://codereview.chromium.org/6649012/diff/2001/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/6649012/diff/2001/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:485: base::ThreadRestrictions::AssertIOAllowed();
Is this one necessary?  Typically we put the assert as close to the call as
possible, so I think we should put it in FlushFile, not here.

[commit-bot: I haz the power, 2011-03-09 20:32:29]

Change committed as 77514

[adamk, 2011-03-09 23:26:45]

On 2011/03/09 04:26:48, willchan wrote:
> I want to add it to more places in there, such as Open() and Close(), but I am
> pretty sure they won't work yet.

FYI, Open already seems to trip thread restrictions on Windows (I fixed this for
BlobURLRequestJob in http://codereview.chromium.org/6612051/).  I agree that
Close will be quit hard. But what would be nice are async versions of Open and
Close: it's annoying to have to use another helper (FileUtilProxy) to make this
stuff async.

[adamk, 2011-03-09 23:59:04]

On Wed, Mar 9, 2011 at 3:50 PM, William Chan (陈智昌) <willchan@chromium.org>wrote:

> On Wed, Mar 9, 2011 at 3:26 PM,  <adamk@chromium.org> wrote:
> > On 2011/03/09 04:26:48, willchan wrote:
> >>
> >> I want to add it to more places in there, such as Open() and Close(),
> but
> >> I am
> >> pretty sure they won't work yet.
> >
> > FYI, Open already seems to trip thread restrictions on Windows (I fixed
> this
> > for
> > BlobURLRequestJob in http://codereview.chromium.org/6612051/).  I agree
> that
> > Close will be quit hard. But what would be nice are async versions of
> Open
> > and
> > Close: it's annoying to have to use another helper (FileUtilProxy) to
> make
> > this
> > stuff async.
>
> It's unsafe to use another helper for something owned by FileStream,
> since it's unsafe to call close() on a file descriptor where
> read()/write() is currently operating in a different thread.
>

Yeah, I only do this for Open() at the moment since FileStream has a
constructor that takes a PlatformFile.  You're quite right that it doesn't
work for Close().


> We should make Close() async. I've got an idea how to do it, but I
> don't have the time right now.


If you have time to describe it sometime, I might be up for putting a change
together.


> >
> > http://codereview.chromium.org/6649012/
> >
>

[willchan no longer on Chromium, 2011-03-10 00:18:35]

On Wed, Mar 9, 2011 at 3:58 PM, Adam Klein <adamk@chromium.org> wrote:
> On Wed, Mar 9, 2011 at 3:50 PM, William Chan (陈智昌) <willchan@chromium.org>
> wrote:
>>
>> On Wed, Mar 9, 2011 at 3:26 PM,  <adamk@chromium.org> wrote:
>> > On 2011/03/09 04:26:48, willchan wrote:
>> >>
>> >> I want to add it to more places in there, such as Open() and Close(),
>> >> but
>> >> I am
>> >> pretty sure they won't work yet.
>> >
>> > FYI, Open already seems to trip thread restrictions on Windows (I fixed
>> > this
>> > for
>> > BlobURLRequestJob in http://codereview.chromium.org/6612051/).  I agree
>> > that
>> > Close will be quit hard. But what would be nice are async versions of
>> > Open
>> > and
>> > Close: it's annoying to have to use another helper (FileUtilProxy) to
>> > make
>> > this
>> > stuff async.
>>
>> It's unsafe to use another helper for something owned by FileStream,
>> since it's unsafe to call close() on a file descriptor where
>> read()/write() is currently operating in a different thread.
>
> Yeah, I only do this for Open() at the moment since FileStream has a
> constructor that takes a PlatformFile.  You're quite right that it doesn't
> work for Close().
>
>>
>> We should make Close() async. I've got an idea how to do it, but I
>> don't have the time right now.
>
> If you have time to describe it sometime, I might be up for putting a change
> together.

I think we need to make the FileStream implementation the same across
platforms. Mainly so we can use WorkerPool instead of IOCompletion in
Windows. At that point, we just make sure all file operations happen
on the WorkerPool (within the FileStream::AsyncContext) when the
FileStream is available. It means we'd have to switch to using the
base::PlatformFile APIs rather than the platform specific APIs we're
currently using, but that's probably good hygiene anyway.

Then Close() would simply mark a bool as closed, and then post the
close operation to the AsyncContext which would execute it after the
previous operation finished executing on the WorkerPool thread. This
would be similar to what we already do for the posix implementation of
FileStream::AsyncContext reads and writes.

>
>>
>> >
>> > http://codereview.chromium.org/6649012/
>> >
>
>

[willchan no longer on Chromium, 2011-03-10 00:26:08]

On Wed, Mar 9, 2011 at 3:26 PM,  <adamk@chromium.org> wrote:
> On 2011/03/09 04:26:48, willchan wrote:
>>
>> I want to add it to more places in there, such as Open() and Close(), but
>> I am
>> pretty sure they won't work yet.
>
> FYI, Open already seems to trip thread restrictions on Windows (I fixed this
> for
> BlobURLRequestJob in http://codereview.chromium.org/6612051/).  I agree that
> Close will be quit hard. But what would be nice are async versions of Open
> and
> Close: it's annoying to have to use another helper (FileUtilProxy) to make
> this
> stuff async.

It's unsafe to use another helper for something owned by FileStream,
since it's unsafe to call close() on a file descriptor where
read()/write() is currently operating in a different thread.

We should make Close() async. I've got an idea how to do it, but I
don't have the time right now.

>
> http://codereview.chromium.org/6649012/
>

[adamk, 2011-03-10 01:51:01]

On Wed, Mar 9, 2011 at 4:12 PM, William Chan (陈智昌) <willchan@chromium.org>wrote:

> On Wed, Mar 9, 2011 at 3:58 PM, Adam Klein <adamk@chromium.org> wrote:
> > On Wed, Mar 9, 2011 at 3:50 PM, William Chan (陈智昌) <
> willchan@chromium.org>
> > wrote:
> >>
> >> On Wed, Mar 9, 2011 at 3:26 PM,  <adamk@chromium.org> wrote:
> >> > On 2011/03/09 04:26:48, willchan wrote:
> >> >>
> >> >> I want to add it to more places in there, such as Open() and Close(),
> >> >> but
> >> >> I am
> >> >> pretty sure they won't work yet.
> >> >
> >> > FYI, Open already seems to trip thread restrictions on Windows (I
> fixed
> >> > this
> >> > for
> >> > BlobURLRequestJob in http://codereview.chromium.org/6612051/).  I
> agree
> >> > that
> >> > Close will be quit hard. But what would be nice are async versions of
> >> > Open
> >> > and
> >> > Close: it's annoying to have to use another helper (FileUtilProxy) to
> >> > make
> >> > this
> >> > stuff async.
> >>
> >> It's unsafe to use another helper for something owned by FileStream,
> >> since it's unsafe to call close() on a file descriptor where
> >> read()/write() is currently operating in a different thread.
> >
> > Yeah, I only do this for Open() at the moment since FileStream has a
> > constructor that takes a PlatformFile.  You're quite right that it
> doesn't
> > work for Close().
> >
> >>
> >> We should make Close() async. I've got an idea how to do it, but I
> >> don't have the time right now.
> >
> > If you have time to describe it sometime, I might be up for putting a
> change
> > together.
>
> I think we need to make the FileStream implementation the same across
> platforms. Mainly so we can use WorkerPool instead of IOCompletion in
> Windows. At that point, we just make sure all file operations happen
> on the WorkerPool (within the FileStream::AsyncContext) when the
> FileStream is available. It means we'd have to switch to using the
> base::PlatformFile APIs rather than the platform specific APIs we're
> currently using, but that's probably good hygiene anyway.
>
> Then Close() would simply mark a bool as closed, and then post the
> close operation to the AsyncContext which would execute it after the
> previous operation finished executing on the WorkerPool thread. This
> would be similar to what we already do for the posix implementation of
> FileStream::AsyncContext reads and writes.


Sounds reasonable enough.  As long as we don't have a problem with losing
the use of native Windows async stuff, this seems pretty straightforward, so
I may be able to find time for it next week.


> >
> >>
> >> >
> >> > http://codereview.chromium.org/6649012/
> >> >
> >
> >
>