filepath.Walk is error-prone when handling errors

[niemeyer]

Right now filepath.Walk will ignore errors by default, but will give the option of
feeding them into an errors channel if desired.

As Philipp Waehnert reported in the ML, though, simply running the walk with a channel
will block the running goroutine if the number of errors exceed the channel buffer size,
unless there's another goroutine reading from the channel.

Then, even if a goroutine is reading from the channel, it's still dangerously tempting
to break down on the first error and stop reading from the channel, which again would
lead to problems due to a goroutine blocked in background. That means one can't
interrupt a running walk from outside in case of errors.

In addition to these issues, errors from the visitor logic itself have to be propagated
through a panic, which has to be correctly integrated with the above details.

Feels like too many things to keep in mind for such a simple problem.

[niemeyer]

Comment 1:

Owner changed to ---.

[robpike]

Comment 2:

Owner changed to @griesemer.

Status changed to Accepted.

[griesemer]

Comment 3:

I see two obvious approaches to fix this:
a) Expand the Visitor with an error handler, and simplify the Walk signature:
type Visitor interface {
    VisitDir(path string, f *os.FileInfo) bool
    VisitFile(path string, f *os.FileInfo)
    Error(os.Error)
}
func Walk(root string, v Visitor)
b) Instead of a channel, provide an error handler to Walk and leave the Visitor as is:
func Walk(root string, v Visitor, error func(os.Error))
Discussion: a) has the advantage of more naturally integrating any kind of error
handling done by a visitor's implementation, but it requires all visitors to implement
an Error method. b) is simply an alternative to what we have now (and existing
implementations can be easily adjusted by providing a function/closure that sends on an
error channel as error handler.
Looking for feedback.

Status changed to WaitingForReply.

[niemeyer]

Comment 4:

I'd personally prefer (a), because it enables the visitor itself to use the same
error handling mechanism by calling v.Error within VisitDir and/or VisitFile if
wanted.
I also suggest we use the signature of Error(path string, os.Error) instead, to
make the interface even and also to enable the erroring logic to use the path
if desired, but that's minor.

[niemeyer]

Comment 5:

Also, just for completeness, there is a third option which is a variation of (a):
c) Use an independent ErrorHandler interface to determine if the visitor
   wants to handle errors or not.
I think (a) is still cleaner, though, as it is an explicit acknowledgement from
the developer that errors are being ignored.

[rsc]

Comment 6:

a sgtm

[niemeyer]

Comment 7:

Candidate CL: http://golang.org/cl/4964067

[niemeyer]

Comment 8:

This issue was closed by revision e5c20dc.

Status changed to Fixed.

[griesemer]

Comment 9:

This issue was closed by revision 49bcc88.

[griesemer]

Comment 10:

Issue re-opened (fix was rolled back
(https://code.google.com/p/go/source/detail?r=678116cefc3d8068dfc615ee70ff4e0cea139446 ).

Status changed to Accepted.

[niemeyer]

Comment 11:

In case someone faces this issue before the new interface is sorted, the
attached file is a drop-in replacement for filepath.Walk implementing the
suggested API, using option (c) above.  It will detect if the visitor is
an error handler or not, so that the standard filepath.Visitor API
continues to work with existing code.

Attachments:

1. walk.go (1949 bytes)

[robpike]

Comment 12:

This issue was closed by revision 4e3b725.

Status changed to Fixed.