Base: WritePlatformFile now retries the operation if

Base: WritePlatformFile now retries the operation if
the OS writes less than expected (for POSIX).

BUG=94161
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98497

[Scott Hess - ex-Googler, 2011-08-25 21:05:23]

LGTM, minor nit on return value.

http://codereview.chromium.org/7745008/diff/1/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/7745008/diff/1/base/platform_file_posix.cc#new...
base/platform_file_posix.cc:173: return bytes_written ? bytes_written : (size ?
rv : 0);
If !bytes_written && !size, then rv<=0, so I think you don't need the latter
conditional operator.

http://codereview.chromium.org/7745008/diff/1/base/platform_file_unittest.cc
File base/platform_file_unittest.cc (left):

http://codereview.chromium.org/7745008/diff/1/base/platform_file_unittest.cc#...
base/platform_file_unittest.cc:56: return total_bytes_written;
I'm kinda happy you're removing this code!

I'm kinda sad that nobody noticed that the problem being solved wasn't
restricted to unit tests.  Is there any possibility that short writes for
WritePlatformFile() were intentional?

I'm nervous about the test for "Wrote more bytes than I asked you to."  I assume
it wouldn't be there unless it happened, but WTF?

[rvargas (doing something else), 2011-08-25 21:28:06]

Thanks

http://codereview.chromium.org/7745008/diff/1/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/7745008/diff/1/base/platform_file_posix.cc#new...
base/platform_file_posix.cc:173: return bytes_written ? bytes_written : (size ?
rv : 0);
On 2011/08/25 21:05:23, shess wrote:
> If !bytes_written && !size, then rv<=0, so I think you don't need the latter
> conditional operator.

I want to return 0 instead of -1 (rv) if !size, and at the same time return the
same error that pwrite returns if there is a failure (not force a -1)

http://codereview.chromium.org/7745008/diff/1/base/platform_file_unittest.cc
File base/platform_file_unittest.cc (left):

http://codereview.chromium.org/7745008/diff/1/base/platform_file_unittest.cc#...
base/platform_file_unittest.cc:56: return total_bytes_written;
On 2011/08/25 21:05:23, shess wrote:
> I'm kinda happy you're removing this code!
> 
> I'm kinda sad that nobody noticed that the problem being solved wasn't
> restricted to unit tests.  Is there any possibility that short writes for
> WritePlatformFile() were intentional?

I looked at all the callers of this code and only one has a loop, but I'm not
changing that one because it is a weird loop that writes in chunks of 1k (no
idea why).

I don't really know why someone would want to ask to write x and be happy if
only y was written... the current callers just fail whatever they are doing.

> I'm nervous about the test for "Wrote more bytes than I asked you to."  I
assume
> it wouldn't be there unless it happened, but WTF?

That beats me too. The first version of this code has that check and there is no
comment on the CL, so I guess it was just extremely defensive code (as in fail
the test if this ever happens). But if it happens, the test will still fail.

[rvargas (doing something else), 2011-08-25 21:32:19]

+Darin, for owner review.

[Scott Hess - ex-Googler, 2011-08-25 21:33:01]

http://codereview.chromium.org/7745008/diff/1/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/7745008/diff/1/base/platform_file_posix.cc#new...
base/platform_file_posix.cc:173: return bytes_written ? bytes_written : (size ?
rv : 0);
On 2011/08/25 21:28:06, rvargas wrote:
> On 2011/08/25 21:05:23, shess wrote:
> > If !bytes_written && !size, then rv<=0, so I think you don't need the latter
> > conditional operator.
> 
> I want to return 0 instead of -1 (rv) if !size, and at the same time return
the
> same error that pwrite returns if there is a failure (not force a -1)

This means that the code changes to always return 0 for size==0, whereas before
it would only return 0 for size==0 if the 0-sized write was successful.  This
might matter if, for instance, someone is testing if the file is writable, but
not writing anything to it.  They should be repremanded, though.

Actually, I just noticed that the current code doesn't even call pwrite() when
size==0.  If this is the right thing to do, you could just handle it before the
loop.

[rvargas (doing something else), 2011-08-25 21:37:50]

http://codereview.chromium.org/7745008/diff/1/base/platform_file_posix.cc
File base/platform_file_posix.cc (right):

http://codereview.chromium.org/7745008/diff/1/base/platform_file_posix.cc#new...
base/platform_file_posix.cc:173: return bytes_written ? bytes_written : (size ?
rv : 0);
On 2011/08/25 21:33:01, shess wrote:
> On 2011/08/25 21:28:06, rvargas wrote:
> > On 2011/08/25 21:05:23, shess wrote:
> > > If !bytes_written && !size, then rv<=0, so I think you don't need the
latter
> > > conditional operator.
> > 
> > I want to return 0 instead of -1 (rv) if !size, and at the same time return
> the
> > same error that pwrite returns if there is a failure (not force a -1)
> 
> This means that the code changes to always return 0 for size==0, whereas
before
> it would only return 0 for size==0 if the 0-sized write was successful.  This
> might matter if, for instance, someone is testing if the file is writable, but
> not writing anything to it.  They should be repremanded, though.
> 
> Actually, I just noticed that the current code doesn't even call pwrite() when
> size==0.  If this is the right thing to do, you could just handle it before
the
> loop.

No, that's not what I meant... fixing it.

[rvargas (doing something else), 2011-08-25 22:59:54]

New version up.

[Scott Hess - ex-Googler, 2011-08-25 23:09:09]

Definitely!  LGTM.

BTW, did you manually test the short-write case at all?  Maybe something like:

ssize_t my_pwrite(int fildes, const void *buf, size_t nbyte, off_t offset) {
  if (nbyte > 1024) nbyte = 1024;
  return pwrite(fildes, buf, nbyte, offset);
}
#define pwrite(f,b,n,o) my_pwrite(f,b,n,o)

Then run the unit tests or hit the trybots?

I don't think it's wrong, I just wanted to make sure the case it was meant to
fix was fixed.  As-is, if it's been happening we're in the dark (maybe it's
associated with really random unexplained stuff).

[Scott Hess - ex-Googler, 2011-08-25 23:10:14]

On 2011/08/25 23:09:09, shess wrote:
> BTW, did you manually test the short-write case at all?  Maybe something like:

Oh, not meant as a blocking suggestion, just checking.

[darin (slow to review), 2011-08-25 23:54:24]

http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h
File base/platform_file.h (right):

http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h#newcod...
base/platform_file.h:126: // on error. Note that this function automatically
retries writing if it is
Is this to match the behavior of Windows?  If so, does that mean that this
change is about ensuring that, on all platforms, all of the given data will be
written unless there is an error?

[rvargas (doing something else), 2011-08-26 00:09:36]

http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h
File base/platform_file.h (right):

http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h#newcod...
base/platform_file.h:126: // on error. Note that this function automatically
retries writing if it is
On 2011/08/25 23:54:24, darin wrote:
> Is this to match the behavior of Windows?  If so, does that mean that this
> change is about ensuring that, on all platforms, all of the given data will be
> written unless there is an error?

Something like that. I have another code review where I'm using
WritePlatformFile, and this issue came afloat. I don't think it makes sense to
ask all users of Write() to have loops to make sure that the operation succeeds,
even if all OSes were behaving the same way regarding signals (actually, if all
OSes were doing it, maybe that would be the familiar thing to do, dunno).

I was just thinking that maybe we should add something like ReadUntilComplete()
to this file to be more symmetric. Maybe in a follow-up cl?

[darin (slow to review), 2011-08-26 05:59:46]

On Thu, Aug 25, 2011 at 5:09 PM, <rvargas@chromium.org> wrote:

>
>
http://codereview.chromium.**org/7745008/diff/10001/base/**platform_file.h<ht...
> File base/platform_file.h (right):
>
> http://codereview.chromium.**org/7745008/diff/10001/base/**
>
platform_file.h#newcode126<http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h#newcode126>
> base/platform_file.h:126: // on error. Note that this function
> automatically retries writing if it is
> On 2011/08/25 23:54:24, darin wrote:
>
>> Is this to match the behavior of Windows?  If so, does that mean that
>>
> this
>
>> change is about ensuring that, on all platforms, all of the given data
>>
> will be
>
>> written unless there is an error?
>>
>
> Something like that. I have another code review where I'm using
> WritePlatformFile, and this issue came afloat. I don't think it makes
> sense to ask all users of Write() to have loops to make sure that the
> operation succeeds, even if all OSes were behaving the same way
> regarding signals (actually, if all OSes were doing it, maybe that would
> be the familiar thing to do, dunno).
>

Yeah, I'm happy with this change.  Given that you want the behavior to be
the same on all plaforms (i.e., to make a best effort to write all given
data
before returning), I think we should just document that instead of
mentioning
the implementation detail of retrying.

-Darin



>
> I was just thinking that maybe we should add something like
> ReadUntilComplete() to this file to be more symmetric. Maybe in a
> follow-up cl?
>
>
>
http://codereview.chromium.**org/7745008/<http://codereview.chromium.org/7745...
>

[rvargas (doing something else), 2011-08-26 21:07:00]

On 2011/08/26 05:59:46, darin wrote:
> On Thu, Aug 25, 2011 at 5:09 PM, <mailto:rvargas@chromium.org> wrote:
> 
> >
> >
>
http://codereview.chromium.**org/7745008/diff/10001/base/**platform_file.h%3C...>
> > File base/platform_file.h (right):
> >
> > http://codereview.chromium.**org/7745008/diff/10001/base/**
> >
>
platform_file.h#newcode126<http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h#newcode126>
> > base/platform_file.h:126: // on error. Note that this function
> > automatically retries writing if it is
> > On 2011/08/25 23:54:24, darin wrote:
> >
> >> Is this to match the behavior of Windows?  If so, does that mean that
> >>
> > this
> >
> >> change is about ensuring that, on all platforms, all of the given data
> >>
> > will be
> >
> >> written unless there is an error?
> >>
> >
> > Something like that. I have another code review where I'm using
> > WritePlatformFile, and this issue came afloat. I don't think it makes
> > sense to ask all users of Write() to have loops to make sure that the
> > operation succeeds, even if all OSes were behaving the same way
> > regarding signals (actually, if all OSes were doing it, maybe that would
> > be the familiar thing to do, dunno).
> >
> 
> Yeah, I'm happy with this change.  Given that you want the behavior to be
> the same on all plaforms (i.e., to make a best effort to write all given
> data
> before returning), I think we should just document that instead of
> mentioning
> the implementation detail of retrying.

Thanks.

I updated the comment with: "Note that this function makes a best effort to
write all data on all platforms."

> 
> -Darin
> 
> 
> 
> >
> > I was just thinking that maybe we should add something like
> > ReadUntilComplete() to this file to be more symmetric. Maybe in a
> > follow-up cl?
> >
> >
> >
>
http://codereview.chromium.**org/7745008/%3Chttp://codereview.chromium.org/77...>
> >

[darin (slow to review), 2011-08-26 21:25:29]

On Fri, Aug 26, 2011 at 2:07 PM, <rvargas@chromium.org> wrote:

> On 2011/08/26 05:59:46, darin wrote:
>
>> On Thu, Aug 25, 2011 at 5:09 PM, <mailto:rvargas@chromium.org> wrote:
>>
>
>  >
>> >
>>
>
> http://codereview.chromium.****org/7745008/diff/10001/base/****
> platform_file.h%3Chttp://coder**eview.chromium.org/7745008/**
>
diff/10001/base/platform_file.**h<http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h>
> >
>
>  > File base/platform_file.h (right):
>> >
>> > http://codereview.chromium.****org/7745008/diff/10001/base/**
>> >
>>
>
> platform_file.h#newcode126<htt**p://codereview.chromium.org/**
>
7745008/diff/10001/base/**platform_file.h#newcode126<http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h#newcode126>
> >
>
>  > base/platform_file.h:126: // on error. Note that this function
>> > automatically retries writing if it is
>> > On 2011/08/25 23:54:24, darin wrote:
>> >
>> >> Is this to match the behavior of Windows?  If so, does that mean that
>> >>
>> > this
>> >
>> >> change is about ensuring that, on all platforms, all of the given data
>> >>
>> > will be
>> >
>> >> written unless there is an error?
>> >>
>> >
>> > Something like that. I have another code review where I'm using
>> > WritePlatformFile, and this issue came afloat. I don't think it makes
>> > sense to ask all users of Write() to have loops to make sure that the
>> > operation succeeds, even if all OSes were behaving the same way
>> > regarding signals (actually, if all OSes were doing it, maybe that would
>> > be the familiar thing to do, dunno).
>> >
>>
>
>  Yeah, I'm happy with this change.  Given that you want the behavior to be
>> the same on all plaforms (i.e., to make a best effort to write all given
>> data
>> before returning), I think we should just document that instead of
>> mentioning
>> the implementation detail of retrying.
>>
>
> Thanks.
>
> I updated the comment with: "Note that this function makes a best effort to
> write all data on all platforms."


Great, thanks!
-Darin


>
>
>
>  -Darin
>>
>
>
>
>  >
>> > I was just thinking that maybe we should add something like
>> > ReadUntilComplete() to this file to be more symmetric. Maybe in a
>> > follow-up cl?
>> >
>> >
>> >
>>
>
> http://codereview.chromium.****org/7745008/%3Chttp://coderevi**
> ew.chromium.org/7745008/ <http://codereview.chromium.org/7745008/>>
>
>  >
>>
>
>
>
>
http://codereview.chromium.**org/7745008/<http://codereview.chromium.org/7745...
>

[rvargas (doing something else), 2011-08-26 21:26:57]

LGTY ? :)

On 2011/08/26 21:25:29, darin wrote:
> On Fri, Aug 26, 2011 at 2:07 PM, <mailto:rvargas@chromium.org> wrote:
> 
> > On 2011/08/26 05:59:46, darin wrote:
> >
> >> On Thu, Aug 25, 2011 at 5:09 PM, <mailto:rvargas@chromium.org> wrote:
> >>
> >
> >  >
> >> >
> >>
> >
> > http://codereview.chromium.****org/7745008/diff/10001/base/****
> > platform_file.h%3Chttp://coder**eview.chromium.org/7745008/**
> >
>
diff/10001/base/platform_file.**h<http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h>
> > >
> >
> >  > File base/platform_file.h (right):
> >> >
> >> > http://codereview.chromium.****org/7745008/diff/10001/base/**
> >> >
> >>
> >
> > platform_file.h#newcode126<htt**p://codereview.chromium.org/**
> >
>
7745008/diff/10001/base/**platform_file.h#newcode126<http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h#newcode126>
> > >
> >
> >  > base/platform_file.h:126: // on error. Note that this function
> >> > automatically retries writing if it is
> >> > On 2011/08/25 23:54:24, darin wrote:
> >> >
> >> >> Is this to match the behavior of Windows?  If so, does that mean that
> >> >>
> >> > this
> >> >
> >> >> change is about ensuring that, on all platforms, all of the given data
> >> >>
> >> > will be
> >> >
> >> >> written unless there is an error?
> >> >>
> >> >
> >> > Something like that. I have another code review where I'm using
> >> > WritePlatformFile, and this issue came afloat. I don't think it makes
> >> > sense to ask all users of Write() to have loops to make sure that the
> >> > operation succeeds, even if all OSes were behaving the same way
> >> > regarding signals (actually, if all OSes were doing it, maybe that would
> >> > be the familiar thing to do, dunno).
> >> >
> >>
> >
> >  Yeah, I'm happy with this change.  Given that you want the behavior to be
> >> the same on all plaforms (i.e., to make a best effort to write all given
> >> data
> >> before returning), I think we should just document that instead of
> >> mentioning
> >> the implementation detail of retrying.
> >>
> >
> > Thanks.
> >
> > I updated the comment with: "Note that this function makes a best effort to
> > write all data on all platforms."
> 
> 
> Great, thanks!
> -Darin
> 
> 
> >
> >
> >
> >  -Darin
> >>
> >
> >
> >
> >  >
> >> > I was just thinking that maybe we should add something like
> >> > ReadUntilComplete() to this file to be more symmetric. Maybe in a
> >> > follow-up cl?
> >> >
> >> >
> >> >
> >>
> >
> > http://codereview.chromium.****org/7745008/%253Chttp://coderevi**
> > ew.chromium.org/7745008/ <http://codereview.chromium.org/7745008/>>
> >
> >  >
> >>
> >
> >
> >
> >
>
http://codereview.chromium.**org/7745008/%3Chttp://codereview.chromium.org/77...>
> >

[darin (slow to review), 2011-08-26 21:27:34]

Yes, sorry... LGTM :)

On Fri, Aug 26, 2011 at 2:26 PM, <rvargas@chromium.org> wrote:

> LGTY ? :)
>
> On 2011/08/26 21:25:29, darin wrote:
>
>  On Fri, Aug 26, 2011 at 2:07 PM, <mailto:rvargas@chromium.org> wrote:
>>
>
>  > On 2011/08/26 05:59:46, darin wrote:
>> >
>> >> On Thu, Aug 25, 2011 at 5:09 PM, <mailto:rvargas@chromium.org> wrote:
>> >>
>> >
>> >  >
>> >> >
>> >>
>> >
>> > http://codereview.chromium.******org/7745008/diff/10001/base/******
>> >
platform_file.h%3Chttp://**coder**eview.chromium.org/**7745008/**<http://eview.chromium.org/7745008/**>
>> >
>>
>
> diff/10001/base/platform_file.****h<http://codereview.**
>
chromium.org/7745008/diff/**10001/base/platform_file.h<http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h>
> >
>
>> > >
>> >
>> >  > File base/platform_file.h (right):
>> >> >
>> >> > http://codereview.chromium.******org/7745008/diff/10001/base/****
>> >> >
>> >>
>> >
>> >
platform_file.h#newcode126<**htt**p://codereview.chromium.**org/**<http://codereview.chromium.org/**>
>> >
>>
>
> 7745008/diff/10001/base/****platform_file.h#newcode126<htt**
> p://codereview.chromium.org/**7745008/diff/10001/base/**
>
platform_file.h#newcode126<http://codereview.chromium.org/7745008/diff/10001/base/platform_file.h#newcode126>
> >
>
>> > >
>> >
>> >  > base/platform_file.h:126: // on error. Note that this function
>> >> > automatically retries writing if it is
>> >> > On 2011/08/25 23:54:24, darin wrote:
>> >> >
>> >> >> Is this to match the behavior of Windows?  If so, does that mean
>> that
>> >> >>
>> >> > this
>> >> >
>> >> >> change is about ensuring that, on all platforms, all of the given
>> data
>> >> >>
>> >> > will be
>> >> >
>> >> >> written unless there is an error?
>> >> >>
>> >> >
>> >> > Something like that. I have another code review where I'm using
>> >> > WritePlatformFile, and this issue came afloat. I don't think it makes
>> >> > sense to ask all users of Write() to have loops to make sure that the
>> >> > operation succeeds, even if all OSes were behaving the same way
>> >> > regarding signals (actually, if all OSes were doing it, maybe that
>> would
>> >> > be the familiar thing to do, dunno).
>> >> >
>> >>
>> >
>> >  Yeah, I'm happy with this change.  Given that you want the behavior to
>> be
>> >> the same on all plaforms (i.e., to make a best effort to write all
>> given
>> >> data
>> >> before returning), I think we should just document that instead of
>> >> mentioning
>> >> the implementation detail of retrying.
>> >>
>> >
>> > Thanks.
>> >
>> > I updated the comment with: "Note that this function makes a best effort
>> to
>> > write all data on all platforms."
>>
>
>
>  Great, thanks!
>> -Darin
>>
>
>
>  >
>> >
>> >
>> >  -Darin
>> >>
>> >
>> >
>> >
>> >  >
>> >> > I was just thinking that maybe we should add something like
>> >> > ReadUntilComplete() to this file to be more symmetric. Maybe in a
>> >> > follow-up cl?
>> >> >
>> >> >
>> >> >
>> >>
>> >
>> > http://codereview.chromium.******org/7745008/%253Chttp://**coderevi**
>>
>> > ew.chromium.org/7745008/
<http://codereview.chromium.**org/7745008/<http://codereview.chromium.org/7745...
>> >>
>> >
>> >  >
>> >>
>> >
>> >
>> >
>> >
>>
>
> http://codereview.chromium.****org/7745008/%3Chttp://coderevi**
> ew.chromium.org/7745008/ <http://codereview.chromium.org/7745008/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/7745008/<http://codereview.chromium.org/7745...
>