code review 4415041: io: clarify that ReadAt shouldn't move the seek offset

io: clarify that ReadAt shouldn't move the seek offset

[bradfitz, 2011-04-13 16:46:17]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-04-13 16:49:54]

This was at least my interpretation.

I always assumed ReadAt was like pread(2), which documents that it doesn't
move the seek offset.

True?


On Wed, Apr 13, 2011 at 9:46 AM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> io: clarify that ReadAt shouldn't move the seek offset
>
> Please review this at http://codereview.appspot.com/4415041/
>
> Affected files:
>  M src/pkg/io/io.go
>
>
> Index: src/pkg/io/io.go
> ===================================================================
> --- a/src/pkg/io/io.go
> +++ b/src/pkg/io/io.go
> @@ -125,8 +125,9 @@
>  // ReaderAt is the interface that wraps the basic ReadAt method.
>  //
>  // ReadAt reads len(p) bytes into p starting at offset off in the
> -// underlying data stream.  It returns the number of bytes
> -// read (0 <= n <= len(p)) and any error encountered.
> +// underlying data stream without changing the underlying seek offset,
> +// if any. It returns the number of bytes read (0 <= n <= len(p)) and
> +// any error encountered.
>  //
>  // Even if ReadAt returns n < len(p),
>  // it may use all of p as scratch space during the call.
>
>
>

[r, 2011-04-13 17:00:14]

LGTM but let others weigh in

[mkrautz, 2011-04-13 17:13:31]

On 2011/04/13 16:49:54, bradfitzgo wrote:
> This was at least my interpretation.
> 
> I always assumed ReadAt was like pread(2), which documents that it doesn't
> move the seek offset.
> 
> True?
> 

I wondered whether this was the case myself some time ago.  However, right now,
os.File's ReadAt on Windows uses syscall.Pread, which on Windows is implemented
using a seek:

http://code.google.com/p/go/source/browse/src/pkg/syscall/syscall_windows.go#250

[r2, 2011-04-13 17:17:51]

On Apr 13, 2011, at 10:13 AM, krautz@gmail.com wrote:

> On 2011/04/13 16:49:54, bradfitzgo wrote:
>> This was at least my interpretation.
> 
>> I always assumed ReadAt was like pread(2), which documents that it
> doesn't
>> move the seek offset.
> 
>> True?
> 
> 
> I wondered whether this was the case myself some time ago.  However,
> right now, os.File's ReadAt on Windows uses syscall.Pread, which on
> Windows is implemented using a seek:
> 
>
http://code.google.com/p/go/source/browse/src/pkg/syscall/syscall_windows.go#250
> 
> 
> http://codereview.appspot.com/4415041/

I'd say that's a bug in the Windows port, which is not to cast aspersions on the
people attempting to emulate Unix semantics on Windows.  It does raise the
question: does Go's os package need to honor the bugs or just explain where they
might arise?

-rob

[bradfitz, 2011-04-13 17:19:29]

On Wed, Apr 13, 2011 at 10:13 AM, <krautz@gmail.com> wrote:

> On 2011/04/13 16:49:54, bradfitzgo wrote:
>
>> This was at least my interpretation.
>>
>
>  I always assumed ReadAt was like pread(2), which documents that it
>>
> doesn't
>
>> move the seek offset.
>>
>
>  True?
>>
>
>
> I wondered whether this was the case myself some time ago.  However,
> right now, os.File's ReadAt on Windows uses syscall.Pread, which on
> Windows is implemented using a seek:
>
>
>
http://code.google.com/p/go/source/browse/src/pkg/syscall/syscall_windows.go#250


But at least it looks like Alex restores the seek offset at the end, which
matches pread's semantics.... roughly.

I believe you should be able to have one goroutine reading an io.Reader and
another goroutine doing ReadAt calls on the same instance without races,
though.

At least read and pread work like that.

[rsc, 2011-04-13 17:56:42]

http://codereview.appspot.com/4415041/diff/4001/src/pkg/io/io.go
File src/pkg/io/io.go (right):

http://codereview.appspot.com/4415041/diff/4001/src/pkg/io/io.go#newcode128
src/pkg/io/io.go:128: // underlying data stream without changing the underlying
seek offset,
I'm not sure this should be so prominent.  I would suggest leaving the original
text and adding, at the end,

// If ReadAt is reading from an data stream with a seek offset,
// ReadAt should not affect nor be affected by the underlying seek offset.

[bradfitz, 2011-04-13 18:12:59]

Hello golang-dev@googlegroups.com, r, mkrautz, r2, rsc (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-04-13 18:14:26]

LGTM

[bradfitz, 2011-04-13 18:18:45]

*** Submitted as http://code.google.com/p/go/source/detail?r=65b2233724dd ***

io: clarify that ReadAt shouldn't move the seek offset

R=r, mkrautz, r2, rsc
CC=golang-dev
http://codereview.appspot.com/4415041

[brainman, 2011-04-18 07:31:55]

On 2011/04/13 17:19:29, bradfitzgo wrote:
> 
> But at least it looks like Alex restores the seek offset at the end, which
> matches pread's semantics.... roughly.

There is a race there. I wasn't even sure if I should implement semantics at the
time I wrote the code.

> I believe you should be able to have one goroutine reading an io.Reader and
> another goroutine doing ReadAt calls on the same instance without races,
> though.

I would rather use lock private to each file to serialize all pread/pwrite calls
for that file. Feels too heavy for such low level function, but I see no other
alternatives at the moment. If no objections, I'll submit a patch.

> At least read and pread work like that.

Do you mean in linux/libc?

Alex