code review 18870043: os: do not return Lstat errors from Readdir

os: do not return Lstat errors from Readdir

This CL restores the Go 1.1.2 semantics for os.File's Readdir method.

The code in Go 1.1.2 was rewritten mainly because it looked buggy.
This new version attempts to be clearer but still provide the 1.1.2 results.

The important diff is not this CL's version against tip but this CL's version
against Go 1.1.2.

Go 1.1.2:

        names, err := f.Readdirnames(n)
        fi = make([]FileInfo, len(names))
        for i, filename := range names {
                fip, err := Lstat(dirname + filename)
                if err == nil {
                        fi[i] = fip
                } else {
                        fi[i] = &fileStat{name: filename}
                }
        }
        return fi, err

This CL:

        names, err := f.Readdirnames(n)
        fi = make([]FileInfo, len(names))
        for i, filename := range names {
                fip, lerr := lstat(dirname + filename)
                if lerr != nil {
                        fi[i] = &fileStat{name: filename}
                        continue
                }
                fi[i] = fip
        }
        return fi, err

The changes from Go 1.1.2 are stylistic, not semantic:
1. Use lstat instead of Lstat, for testing (done before this CL).
2. Make error handling in loop body look more like an error case.
3. Use separate error variable name in loop body, to be clear
   we are not trying to influence the final return result.

Fixes issue 6656.
Fixes issue 6680.

[rsc, 2013-10-29 00:41:35]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-10-29 01:00:17]

I prefer the semantics at tip.

This way (the Go 1.1 way) silently throws away errors.  You have to go
check fi.Sys() on each element to see if it's nil, but you still don't know
the error.

I would rather fix filepath.Walk.

If we do anything to Readdir at this point, I'd make it just silently skip
over os.IsNotExist errors in the second pass.  But that's not enough of a
fix.




On Mon, Oct 28, 2013 at 5:41 PM, <rsc@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> os: do not return Lstat errors from Readdir
>
> This CL restores the Go 1.1.2 semantics for os.File's Readdir method.
>
> The code in Go 1.1.2 was rewritten mainly because it looked buggy.
> This new version attempts to be clearer but still provide the 1.1.2
> results.
>
> The important diff is not this CL's version against tip but this CL's
> version
> against Go 1.1.2.
>
> Go 1.1.2:
>
>         names, err := f.Readdirnames(n)
>         fi = make([]FileInfo, len(names))
>         for i, filename := range names {
>                 fip, err := Lstat(dirname + filename)
>                 if err == nil {
>                         fi[i] = fip
>                 } else {
>                         fi[i] = &fileStat{name: filename}
>                 }
>         }
>         return fi, err
>
> This CL:
>
>         names, err := f.Readdirnames(n)
>         fi = make([]FileInfo, len(names))
>         for i, filename := range names {
>                 fip, lerr := lstat(dirname + filename)
>                 if lerr != nil {
>                         fi[i] = &fileStat{name: filename}
>                         continue
>                 }
>                 fi[i] = fip
>         }
>         return fi, err
>
> The changes from Go 1.1.2 are stylistic, not semantic:
> 1. Use lstat instead of Lstat, for testing (done before this CL).
> 2. Make error handling in loop body look more like an error case.
> 3. Use separate error variable name in loop body, to be clear
>    we are not trying to influence the final return result.
>
> Fixes issue 6656.
> Fixes issue 6680.
>
> Please review this at
https://codereview.appspot.**com/18870043/<https://codereview.appspot.com/188...
>
> Affected files (+5, -8 lines):
>   M src/pkg/os/file_unix.go
>   M src/pkg/os/os_unix_test.go
>
>
> Index: src/pkg/os/file_unix.go
> ==============================**==============================**=======
> --- a/src/pkg/os/file_unix.go
> +++ b/src/pkg/os/file_unix.go
> @@ -165,14 +165,11 @@
>         fi = make([]FileInfo, len(names))
>         for i, filename := range names {
>                 fip, lerr := lstat(dirname + filename)
> -               if lerr == nil {
> -                       fi[i] = fip
> -               } else {
> +               if lerr != nil {
>                         fi[i] = &fileStat{name: filename}
> -                       if err == nil {
> -                               err = lerr
> -                       }
> +                       continue
>                 }
> +               fi[i] = fip
>         }
>         return fi, err
>  }
> Index: src/pkg/os/os_unix_test.go
> ==============================**==============================**=======
> --- a/src/pkg/os/os_unix_test.go
> +++ b/src/pkg/os/os_unix_test.go
> @@ -92,8 +92,8 @@
>         defer func() { *LstatP = Lstat }()
>
>         dirs, err := handle.Readdir(-1)
> -       if err != ErrInvalid {
> -               t.Fatalf("Expected Readdir to return ErrInvalid, got %v",
> err)
> +       if err != nil {
> +               t.Fatalf("Expected Readdir to return no error, got %v",
> err)
>         }
>         foundfail := false
>         for _, dir := range dirs {
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>

[rsc, 2013-10-29 01:25:38]

Making Go 1.2 Readdir behave differently from Go 1.1 Readdir has already
been demonstrated to cause a bug in existing code (filepath.Walk). What
other code that we don't know about would it also break?

We should make Go 1.2 Readdir match Go 1.1 Readdir. We don't have a blank
slate here.

[bradfitz, 2013-10-29 01:26:18]

Then document it.
 On Oct 28, 2013 6:25 PM, "Russ Cox" <rsc@golang.org> wrote:

> Making Go 1.2 Readdir behave differently from Go 1.1 Readdir has already
> been demonstrated to cause a bug in existing code (filepath.Walk). What
> other code that we don't know about would it also break?
>
> We should make Go 1.2 Readdir match Go 1.1 Readdir. We don't have a blank
> slate here.
>
>

[rsc, 2013-10-29 01:28:11]

   If n <= 0, Readdir returns all the FileInfo from the directory in a
    single slice. In this case, if Readdir succeeds (reads all the way to
    the end of the directory), it returns the slice and a nil error. If it
    encounters an error before the end of the directory, Readdir returns the
    FileInfo read until that point and a non-nil error.

The case in question here is where Readdir has read all the way to the
directory. The new code (in this CL) matches the comment better than the
old code (changed since Go 1.1).

[bradfitz, 2013-10-29 01:41:13]

On Mon, Oct 28, 2013 at 6:28 PM, Russ Cox <rsc@golang.org> wrote:

>    If n <= 0, Readdir returns all the FileInfo from the directory in a
>     single slice. In this case, if Readdir succeeds (reads all the way to
>     the end of the directory), it returns the slice and a nil error. If it
>     encounters an error before the end of the directory, Readdir returns
> the
>     FileInfo read until that point and a non-nil error.
>
> The case in question here is where Readdir has read all the way to the
> directory. The new code (in this CL) matches the comment better than the
> old code (changed since Go 1.1).
>

Is the reader supposed to know or assume that "reads all the way to the end
of the directory" means names only doesn't include reading os.FileInfo?
 They shouldn't, because that's not how it works in file_windows.go.

I would say something like:

"If n <= 0 ......  If all the names of the directory are successfully read,
but one or more files' FileInfo can't be retrieved due to errors, stub
os.FileInfo values (implementing only the Name method) are returned in the
slice instead and no error is returned from Readdir."

Then at least we don't "fix" this again in the future, and it explains why
we're throwing away lstat errors.

[rsc, 2013-10-29 02:17:38]

The implementation is OS-dependent. The doc comment is not. I don't want
Unix details leaking into this.

This was fine - as were the docs - in Go 1.1.
Then someone submitted a CL to "clean it up", which changed the semantics.
This CL is putting the old semantics back and updating a test so that we
ensure the semantics are kept.

Russ

[bradfitz, 2013-10-29 02:26:40]

I think the Go 1.1 behavior was bad, silently discarding Lstat errors.  If
lstat is merely an implementation detail of Readdir, then Readdir (as it's
capitalized in the doc comment: not the lowercase C function) did not read
"all the way to the end".  The implementation failed before that point.

IMO the Go 1.1 doc comment and implementation aren't good.

I would accept changing it back for compatibility reasons, but not because
it's correct.  Go style isn't to silently throw away errors.




On Mon, Oct 28, 2013 at 7:17 PM, Russ Cox <rsc@golang.org> wrote:

> The implementation is OS-dependent. The doc comment is not. I don't want
> Unix details leaking into this.
>
> This was fine - as were the docs - in Go 1.1.
> Then someone submitted a CL to "clean it up", which changed the semantics.
> This CL is putting the old semantics back and updating a test so that we
> ensure the semantics are kept.
>
> Russ
>

[rsc, 2013-10-29 02:30:39]

On Mon, Oct 28, 2013 at 10:26 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> I think the Go 1.1 behavior was bad, silently discarding Lstat errors.  If
> lstat is merely an implementation detail of Readdir, then Readdir (as it's
> capitalized in the doc comment: not the lowercase C function) did not read
> "all the way to the end".  The implementation failed before that point.
>

Readdir did read the *directory* all the way to the end (during the call to
Readdirnames). What it failed to do was obtain information about one of
those names, and it signaled that by putting a stub value into the list.
That's reasonable: better to record one partial result in the list than to
return an error impugning the entire list.

Readdir fundamentally is not an atomic operation on Unix. I believe this is
the best we can do, and it is the right implementation.

Russ

[bradfitz, 2013-10-29 02:37:05]

On Mon, Oct 28, 2013 at 7:30 PM, Russ Cox <rsc@golang.org> wrote:

> On Mon, Oct 28, 2013 at 10:26 PM, Brad Fitzpatrick
<bradfitz@golang.org>wrote:
>
>> I think the Go 1.1 behavior was bad, silently discarding Lstat errors.
>>  If lstat is merely an implementation detail of Readdir, then Readdir (as
>> it's capitalized in the doc comment: not the lowercase C function) did not
>> read "all the way to the end".  The implementation failed before that point.
>>
>
> Readdir did read the *directory* all the way to the end (during the call
> to Readdirnames).
>

Speak to me like a Go user, and stop assuming I know C or Unix or any
system calls.


> What it failed to do was obtain information about one of those names, and
> it signaled that by putting a stub value into the list. That's reasonable:
> better to record one partial result in the list than to return an error
> impugning the entire list.
>

And if I call ioutil.ReadAll and it can't read some bytes, should it just
return zero bytes instead, rather than returning an error?

I'm not buying it at all.  I want to believe you and see your side, but I
just can't here.  We're throwing away an error and making up data.  I
believe we can and should skip over ENOENT, but nothing else.

Readdir fundamentally is not an atomic operation on Unix. I believe this is
> the best we can do, and it is the right implementation.
>

I understand it's not atomic, but I don't think this is the right
implementation.

[rsc, 2013-10-29 02:39:23]

For Go 1.3 I don't mind thinking about skipping over IsNotExist(lerr) and
returning other errors, if people can make an argument that those errors
are so rare they should be reported.

But it's too late for that now.

For now we should preserve the Go 1.1 behavior, which is what this CL does.

Russ

[bradfitz, 2013-10-29 15:14:24]

Even after sleeping on it, I still think we should just fix it.  Otherwise
what's the point of having stabilization periods and encouraging users to
use rc releases and report bugs?

We have another month still. We know the Go 1.1 way is good enough and it's
easy to do that but it doesn't make forward progress towards a better Go.



On Mon, Oct 28, 2013 at 7:39 PM, Russ Cox <rsc@golang.org> wrote:

> For Go 1.3 I don't mind thinking about skipping over IsNotExist(lerr) and
> returning other errors, if people can make an argument that those errors
> are so rare they should be reported.
>
> But it's too late for that now.
>
> For now we should preserve the Go 1.1 behavior, which is what this CL does.
>
> Russ
>

[rsc, 2013-10-29 15:35:11]

It is too late to be designing new semantics. This late in the Go 1.2
release process there are really only two choices:

1. Leave things as they are now (making Go 1.2 different from Go 1.1).
2. Put things back like they were in Go 1.1.

These are the two things we have significant experience with. Introducing
some other semantics invalidates the release candidate testing to this
point.

We already know that #1 breaks existing code. That leaves #2.

I am happy to reopen this during the ordinary Go 1.3 development window,
but not now.

Russ

[bradfitz, 2013-10-29 15:38:39]

LGTM  :/

I'll open some bugs.



On Tue, Oct 29, 2013 at 8:35 AM, Russ Cox <rsc@golang.org> wrote:

> It is too late to be designing new semantics. This late in the Go 1.2
> release process there are really only two choices:
>
> 1. Leave things as they are now (making Go 1.2 different from Go 1.1).
> 2. Put things back like they were in Go 1.1.
>
> These are the two things we have significant experience with. Introducing
> some other semantics invalidates the release candidate testing to this
> point.
>
> We already know that #1 breaks existing code. That leaves #2.
>
> I am happy to reopen this during the ordinary Go 1.3 development window,
> but not now.
>
> Russ
>

[rsc, 2013-10-29 15:50:45]

*** Submitted as https://code.google.com/p/go/source/detail?r=eca0ca43a863 ***

os: do not return Lstat errors from Readdir

This CL restores the Go 1.1.2 semantics for os.File's Readdir method.

The code in Go 1.1.2 was rewritten mainly because it looked buggy.
This new version attempts to be clearer but still provide the 1.1.2 results.

The important diff is not this CL's version against tip but this CL's version
against Go 1.1.2.

Go 1.1.2:

        names, err := f.Readdirnames(n)
        fi = make([]FileInfo, len(names))
        for i, filename := range names {
                fip, err := Lstat(dirname + filename)
                if err == nil {
                        fi[i] = fip
                } else {
                        fi[i] = &fileStat{name: filename}
                }
        }
        return fi, err

This CL:

        names, err := f.Readdirnames(n)
        fi = make([]FileInfo, len(names))
        for i, filename := range names {
                fip, lerr := lstat(dirname + filename)
                if lerr != nil {
                        fi[i] = &fileStat{name: filename}
                        continue
                }
                fi[i] = fip
        }
        return fi, err

The changes from Go 1.1.2 are stylistic, not semantic:
1. Use lstat instead of Lstat, for testing (done before this CL).
2. Make error handling in loop body look more like an error case.
3. Use separate error variable name in loop body, to be clear
   we are not trying to influence the final return result.

Fixes issue 6656.
Fixes issue 6680.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/18870043

[awalterschulze, 2013-10-29 15:59:48]

On 2013/10/29 15:50:45, rsc wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=eca0ca43a863 ***
> 
> os: do not return Lstat errors from Readdir
> 
> This CL restores the Go 1.1.2 semantics for os.File's Readdir method.
> 
> The code in Go 1.1.2 was rewritten mainly because it looked buggy.
> This new version attempts to be clearer but still provide the 1.1.2 results.
> 
> The important diff is not this CL's version against tip but this CL's version
> against Go 1.1.2.
> 
> Go 1.1.2:
> 
>         names, err := f.Readdirnames(n)
>         fi = make([]FileInfo, len(names))
>         for i, filename := range names {
>                 fip, err := Lstat(dirname + filename)
>                 if err == nil {
>                         fi[i] = fip
>                 } else {
>                         fi[i] = &fileStat{name: filename}
>                 }
>         }
>         return fi, err
> 
> This CL:
> 
>         names, err := f.Readdirnames(n)
>         fi = make([]FileInfo, len(names))
>         for i, filename := range names {
>                 fip, lerr := lstat(dirname + filename)
>                 if lerr != nil {
>                         fi[i] = &fileStat{name: filename}
>                         continue
>                 }
>                 fi[i] = fip
>         }
>         return fi, err
> 
> The changes from Go 1.1.2 are stylistic, not semantic:
> 1. Use lstat instead of Lstat, for testing (done before this CL).
> 2. Make error handling in loop body look more like an error case.
> 3. Use separate error variable name in loop body, to be clear
>    we are not trying to influence the final return result.
> 
> Fixes issue 6656.
> Fixes issue 6680.
> 
> R=golang-dev, bradfitz
> CC=golang-dev
> https://codereview.appspot.com/18870043

Please update the docs if that is your fix

http://golang.org/pkg/os/#File.Readdir
"If it encounters an error before the end of the directory, Readdir returns the
FileInfo read until that point and a non-nil error."

[awalterschulze, 2013-10-29 16:06:09]

On 2013/10/29 15:59:48, awalterschulze wrote:
> On 2013/10/29 15:50:45, rsc wrote:
> > *** Submitted as https://code.google.com/p/go/source/detail?r=eca0ca43a863
***
> > 
> > os: do not return Lstat errors from Readdir
> > 
> > This CL restores the Go 1.1.2 semantics for os.File's Readdir method.
> > 
> > The code in Go 1.1.2 was rewritten mainly because it looked buggy.
> > This new version attempts to be clearer but still provide the 1.1.2 results.
> > 
> > The important diff is not this CL's version against tip but this CL's
version
> > against Go 1.1.2.
> > 
> > Go 1.1.2:
> > 
> >         names, err := f.Readdirnames(n)
> >         fi = make([]FileInfo, len(names))
> >         for i, filename := range names {
> >                 fip, err := Lstat(dirname + filename)
> >                 if err == nil {
> >                         fi[i] = fip
> >                 } else {
> >                         fi[i] = &fileStat{name: filename}
> >                 }
> >         }
> >         return fi, err
> > 
> > This CL:
> > 
> >         names, err := f.Readdirnames(n)
> >         fi = make([]FileInfo, len(names))
> >         for i, filename := range names {
> >                 fip, lerr := lstat(dirname + filename)
> >                 if lerr != nil {
> >                         fi[i] = &fileStat{name: filename}
> >                         continue
> >                 }
> >                 fi[i] = fip
> >         }
> >         return fi, err
> > 
> > The changes from Go 1.1.2 are stylistic, not semantic:
> > 1. Use lstat instead of Lstat, for testing (done before this CL).
> > 2. Make error handling in loop body look more like an error case.
> > 3. Use separate error variable name in loop body, to be clear
> >    we are not trying to influence the final return result.
> > 
> > Fixes issue 6656.
> > Fixes issue 6680.
> > 
> > R=golang-dev, bradfitz
> > CC=golang-dev
> > https://codereview.appspot.com/18870043
> 
> Please update the docs if that is your fix
> 
> http://golang.org/pkg/os/#File.Readdir
> "If it encounters an error before the end of the directory, Readdir returns
the
> FileInfo read until that point and a non-nil error."

Ok after reading these comments.
Maybe you can just open some bugs.
But it would have been really cool to fix, since all the work is done.