Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: Readdir returns error if file disappears during call #6680

Closed
rsc opened this issue Oct 28, 2013 · 12 comments
Closed

os: Readdir returns error if file disappears during call #6680

rsc opened this issue Oct 28, 2013 · 12 comments
Assignees
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Oct 28, 2013

os.File's Readdir method calls Readdirnames, retrieves a list of names, and then calls
os.Lstat on each one to get a FileInfo. It can happen that a directory entry appears in
the Readdirnames list but then the Lstat fails, if some other program deletes the
corresponding file between the two operations.

The Go 1.1 Readdir code handled this with some confused code that had the effect of
putting the name in the list in a fake FileInfo with no other information. The Go 1.2
Readdir code is cleaned up a bit and returns an error in this case. 

It is not clear that returning the error is the right thing to do in general - Readdir
can instead return the contents that were still there the second time through. It is
especially not clear given that the new behavior does not match Go 1.1's behavior.

If we fix Readdir we do not need CL 16100043.
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 28, 2013

Comment 1:

I agree that on the second pass, if the error is simply ENOENT, we don't have to add it
to the list and don't need to return an error from Readdir.
But if the error is something more serious (corrupt filesystem, or something), then
Readdir should return an error.
And if readdir does return an error, I think we're back to the same problem:
filepath.Walk used to (and is documented to) let you see a file you can't stat.
@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Oct 29, 2013

Comment 2:

Readdir works better than I expected. It returns a []FileInfo for every name returned by
Readdirnames, with a dummy entry for names for which Lstat failed. In the case of such a
failed Lstat, Readdir also returns the first such error, but it still returns the full
[]FileInfo too.
The change from Go 1.1 is that Go 1.1 returned nil even if there was a failed Lstat. I
will reinstate that behavior.
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 29, 2013

Comment 3:

I don't think that's a "fix".
Are you proposing changing it back to keep bug-for-bug compatibility with Go 1.1, or to
fix filepath.Walk?
If the former, at least be very explicit about that and update the documentation to say
how it now works, now and forever.  (sadly)
I would prefer we just keep the new fixed behavior and adjust filepath.Walk accordingly.
@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Oct 29, 2013

Comment 4:

It's not bug-for-bug compatibility. Readdir reports errors from reading the
directory, not from Stat.
@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Oct 29, 2013

Comment 5:

This issue was closed by revision ef805fe.

Status changed to Fixed.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 29, 2013

Comment 6:

Re-opening for Go1.3.
Re-opening for Go1.3.
See discussion at https://golang.org/cl/18870043
Proposal is to both fix os.File.Readdir to skip over Lstat ENOENT errors in the second
pass on Unix and not ignore other Lstat errors, then change filepath.Walk to use
Readdirnames + individual Lstat calls, as per the WalkFunc documentation.

Labels changed: added go1.3, removed go1.2.

Owner changed to @bradfitz.

Status changed to Accepted.

@adg

This comment has been minimized.

Copy link
Contributor

@adg adg commented Nov 1, 2013

Comment 7:

This issue was closed by revision b7ce8f68901f.

Status changed to Fixed.

@adg

This comment has been minimized.

Copy link
Contributor

@adg adg commented Nov 1, 2013

Comment 8:

Status changed to Accepted.

@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Dec 4, 2013

Comment 9:

Labels changed: added release-go1.3.

@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Dec 4, 2013

Comment 10:

Labels changed: removed go1.3.

@rsc

This comment has been minimized.

Copy link
Contributor Author

@rsc rsc commented Dec 4, 2013

Comment 11:

Labels changed: added repo-main.

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Dec 17, 2013

Comment 12:

This issue was closed by revision 6a1a217.

Status changed to Fixed.

@rsc rsc added fixed labels Dec 17, 2013
@rsc rsc added this to the Go1.3 milestone Apr 14, 2015
@rsc rsc removed the release-go1.3 label Apr 14, 2015
adg added a commit that referenced this issue May 11, 2015
««« CL 18870043 / 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 #6656.
Fixes #6680.

R=golang-dev, bradfitz
CC=golang-dev
https://golang.org/cl/18870043
»»»

R=golang-dev
CC=golang-dev
https://golang.org/cl/20110045
@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.