Implement Read/WriteAtCurrentPos correctly

Implement Read/WriteAtCurrentPos correctly

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

[rvargas (doing something else), 2014-03-27 17:41:31]

Looks good but you need an owner anyway so adding Mark.

[hashimoto, 2014-03-31 02:56:03]

Mark is ooo.
Nico, could you take a look at this change as an owner?

[Nico, 2014-03-31 04:10:37]

lgtm

Wow. Is it possible to test this?

[Nico, 2014-03-31 04:11:13]

Sorry, meant to hit "publish drafts" but hit tab once too often and hit "quick
lgtm" instead. Please ignore the "lgtm" line for now :-)

[hashimoto, 2014-03-31 04:20:04]

On 2014/03/31 04:11:13, Nico wrote:
> Sorry, meant to hit "publish drafts" but hit tab once too often and hit "quick
> lgtm" instead. Please ignore the "lgtm" line for now :-)

For read, it's possible to write a test with a real thread which performs
write() slowly, where the return value of read() on the main thread is smaller
than the buffer size, but not reaching EOF.
For write, I cannot come up with an idea to write such tests.
Should we add tests only for read?

[Nico, 2014-03-31 04:33:10]

rvargas, can you help coming up with a test?

(Why does the test have to use threads at all? This looks like a fix that's
relevant in single-thread use too to my uniformed eyes.)

[hashimoto, 2014-03-31 04:39:06]

On 2014/03/31 04:33:10, Nico wrote:
> rvargas, can you help coming up with a test?
> 
> (Why does the test have to use threads at all? This looks like a fix that's
> relevant in single-thread use too to my uniformed eyes.)

I'd use threads to implement the said test because I couldn't come up with a way
to produce a situation where read() returns a smaller value than the buffer size
before reaching EOF.
If it can be done without real threads, I'd be happy to do that way.

[rvargas (doing something else), 2014-03-31 19:18:57]

On 2014/03/31 04:39:06, hashimoto wrote:
> On 2014/03/31 04:33:10, Nico wrote:
> > rvargas, can you help coming up with a test?
> > 
> > (Why does the test have to use threads at all? This looks like a fix that's
> > relevant in single-thread use too to my uniformed eyes.)
> 
> I'd use threads to implement the said test because I couldn't come up with a
way
> to produce a situation where read() returns a smaller value than the buffer
size
> before reaching EOF.
> If it can be done without real threads, I'd be happy to do that way.

Nico, could we land this as is while we come up with a test? We are waiting for
a merge to M35.

As for the test, this function is meant to be used with things like files on
disk, which should always be able to read whatever was requested (as long as the
file is large enough), as opposed to say, streams. The problem was of course
that signals can interrupt the operation before completion... but for that now
we have the retry macro.

I _think_ the system can decide to return part of the data from disk if there's
a lot of disk activity and doing another read from disk will take time (maybe a
fragmented file?), but I'm mostly guessing. What I know is that we used to have
tests outside of base that required looping to avoid flakiness in loaded
systems.

I don't think we want to write a test with something like a pipe (but we could)
because that is not the intended use case. On the other hand, that would be the
simplest test... overloading the system just to see if it split reads in order
to exercise this path seems less than ideal.

[Nico, 2014-03-31 19:27:37]

On 2014/03/31 19:18:57, rvargas wrote:
> On 2014/03/31 04:39:06, hashimoto wrote:
> > On 2014/03/31 04:33:10, Nico wrote:
> > > rvargas, can you help coming up with a test?
> > > 
> > > (Why does the test have to use threads at all? This looks like a fix
that's
> > > relevant in single-thread use too to my uniformed eyes.)
> > 
> > I'd use threads to implement the said test because I couldn't come up with a
> way
> > to produce a situation where read() returns a smaller value than the buffer
> size
> > before reaching EOF.
> > If it can be done without real threads, I'd be happy to do that way.
> 
> Nico, could we land this as is while we come up with a test? We are waiting
for
> a merge to M35.

Ok, lgtm

> As for the test, this function is meant to be used with things like files on
> disk, which should always be able to read whatever was requested (as long as
the
> file is large enough), as opposed to say, streams. The problem was of course
> that signals can interrupt the operation before completion... but for that now
> we have the retry macro.

A pipe sounds like a good suggestion for a test.

> I _think_ the system can decide to return part of the data from disk if
there's
> a lot of disk activity and doing another read from disk will take time (maybe
a
> fragmented file?), but I'm mostly guessing. What I know is that we used to
have
> tests outside of base that required looping to avoid flakiness in loaded
> systems.

At least on posix, `man 2 read` says "The system guarantees to read the number
of bytes requested if the descriptor references a normal file that has that many
bytes left before the end-of-file, but in no other case.".

> I don't think we want to write a test with something like a pipe (but we
could)
> because that is not the intended use case. On the other hand, that would be
the
> simplest test... overloading the system just to see if it split reads in order
> to exercise this path seems less than ideal.

[rvargas (doing something else), 2014-03-31 19:47:42]

On 2014/03/31 19:27:37, Nico wrote:
> On 2014/03/31 19:18:57, rvargas wrote:
> > On 2014/03/31 04:39:06, hashimoto wrote:
> > > On 2014/03/31 04:33:10, Nico wrote:
> > > > rvargas, can you help coming up with a test?
> > > > 
> > > > (Why does the test have to use threads at all? This looks like a fix
> that's
> > > > relevant in single-thread use too to my uniformed eyes.)
> > > 
> > > I'd use threads to implement the said test because I couldn't come up with
a
> > way
> > > to produce a situation where read() returns a smaller value than the
buffer
> > size
> > > before reaching EOF.
> > > If it can be done without real threads, I'd be happy to do that way.
> > 
> > Nico, could we land this as is while we come up with a test? We are waiting
> for
> > a merge to M35.
> 
> Ok, lgtm
> 
> > As for the test, this function is meant to be used with things like files on
> > disk, which should always be able to read whatever was requested (as long as
> the
> > file is large enough), as opposed to say, streams. The problem was of course
> > that signals can interrupt the operation before completion... but for that
now
> > we have the retry macro.
> 
> A pipe sounds like a good suggestion for a test.
> 
> > I _think_ the system can decide to return part of the data from disk if
> there's
> > a lot of disk activity and doing another read from disk will take time
(maybe
> a
> > fragmented file?), but I'm mostly guessing. What I know is that we used to
> have
> > tests outside of base that required looping to avoid flakiness in loaded
> > systems.
> 
> At least on posix, `man 2 read` says "The system guarantees to read the number
> of bytes requested if the descriptor references a normal file that has that
many
> bytes left before the end-of-file, but in no other case.".

Maybe we should remove the NoBestEffort versions then?

> 
> > I don't think we want to write a test with something like a pipe (but we
> could)
> > because that is not the intended use case. On the other hand, that would be
> the
> > simplest test... overloading the system just to see if it split reads in
order
> > to exercise this path seems less than ideal.

[rvargas (doing something else), 2014-03-31 19:47:55]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-03-31 19:48:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/213473007/1

[commit-bot: I haz the power, 2014-03-31 22:18:12]

Change committed as 260673

[hashimoto, 2014-04-04 06:38:49]

On 2014/03/31 19:47:42, rvargas wrote:
> On 2014/03/31 19:27:37, Nico wrote:
> > On 2014/03/31 19:18:57, rvargas wrote:
> > > On 2014/03/31 04:39:06, hashimoto wrote:
> > > > On 2014/03/31 04:33:10, Nico wrote:
> > > > > rvargas, can you help coming up with a test?
> > > > > 
> > > > > (Why does the test have to use threads at all? This looks like a fix
> > that's
> > > > > relevant in single-thread use too to my uniformed eyes.)
> > > > 
> > > > I'd use threads to implement the said test because I couldn't come up
with
> a
> > > way
> > > > to produce a situation where read() returns a smaller value than the
> buffer
> > > size
> > > > before reaching EOF.
> > > > If it can be done without real threads, I'd be happy to do that way.
> > > 
> > > Nico, could we land this as is while we come up with a test? We are
waiting
> > for
> > > a merge to M35.
> > 
> > Ok, lgtm
> > 
> > > As for the test, this function is meant to be used with things like files
on
> > > disk, which should always be able to read whatever was requested (as long
as
> > the
> > > file is large enough), as opposed to say, streams. The problem was of
course
> > > that signals can interrupt the operation before completion... but for that
> now
> > > we have the retry macro.
> > 
> > A pipe sounds like a good suggestion for a test.
> > 
> > > I _think_ the system can decide to return part of the data from disk if
> > there's
> > > a lot of disk activity and doing another read from disk will take time
> (maybe
> > a
> > > fragmented file?), but I'm mostly guessing. What I know is that we used to
> > have
> > > tests outside of base that required looping to avoid flakiness in loaded
> > > systems.
> > 
> > At least on posix, `man 2 read` says "The system guarantees to read the
number
> > of bytes requested if the descriptor references a normal file that has that
> many
> > bytes left before the end-of-file, but in no other case.".
> 
> Maybe we should remove the NoBestEffort versions then?
I'm a bit worried about this movement.
I could find the "The system guarantees..." sentence in the man page of BSD, but
could not in the one of Linux.
> 
> > 
> > > I don't think we want to write a test with something like a pipe (but we
> > could)
> > > because that is not the intended use case. On the other hand, that would
be
> > the
> > > simplest test... overloading the system just to see if it split reads in
> order
> > > to exercise this path seems less than ideal.