filesystem: fix some error handling (#36856)

walkdir: fix thrown error location and types and performance
readdir: fix thrown error code on some OS
seek: fix thrown error code on Windows

Author: vtjnash

base/file.jl

@@ -777,7 +777,7 @@ function readdir(dir::AbstractString; join::Bool=false, sort::Bool=true)
     # defined in sys.c, to call uv_fs_readdir, which sets errno on error.
     err = ccall(:uv_fs_scandir, Int32, (Ptr{Cvoid}, Ptr{UInt8}, Cstring, Cint, Ptr{Cvoid}),
                 C_NULL, uv_readdir_req, dir, 0, C_NULL)
-    err < 0 && throw(SystemError("unable to read directory $dir", -err))
+    err < 0 && throw(_UVError("readdir", err, "with ", repr(dir)))
 
     # iterate the listing into entries
     entries = String[]
@@ -804,10 +804,9 @@ readdir(; join::Bool=false, sort::Bool=true) =
 Return an iterator that walks the directory tree of a directory.
 The iterator returns a tuple containing `(rootpath, dirs, files)`.
 The directory tree can be traversed top-down or bottom-up.
-If `walkdir` encounters a [`SystemError`](@ref)
-it will rethrow the error by default.
+If `walkdir` or `stat` encounters a `IOError` it will rethrow the error by default.
 A custom error handling function can be provided through `onerror` keyword argument.
-`onerror` is called with a `SystemError` as argument.
+`onerror` is called with a `IOError` as argument.
 
 # Examples
 ```julia
@@ -839,48 +838,45 @@ julia> (root, dirs, files) = first(itr)
 ```
 """
 function walkdir(root; topdown=true, follow_symlinks=false, onerror=throw)
-    content = nothing
-    try
-        content = readdir(root)
-    catch err
-        isa(err, SystemError) || throw(err)
-        onerror(err)
-        # Need to return an empty closed channel to skip the current root folder
-        chnl = Channel(0)
-        close(chnl)
-        return chnl
-    end
-    dirs = Vector{eltype(content)}()
-    files = Vector{eltype(content)}()
-    for name in content
-        path = joinpath(root, name)
-
-        # If we're not following symlinks, then treat all symlinks as files
-        if (!follow_symlinks && islink(path)) || !isdir(path)
-            push!(files, name)
-        else
-            push!(dirs, name)
+    function _walkdir(chnl, root)
+        tryf(f, p) = try
+                f(p)
+            catch err
+                isa(err, IOError) || rethrow()
+                try
+                    onerror(err)
+                catch err2
+                    close(chnl, err2)
+                end
+                return
+            end
+        content = tryf(readdir, root)
+        content === nothing && return
+        dirs = Vector{eltype(content)}()
+        files = Vector{eltype(content)}()
+        for name in content
+            path = joinpath(root, name)
+
+            # If we're not following symlinks, then treat all symlinks as files
+            if (!follow_symlinks && something(tryf(islink, path), true)) || !something(tryf(isdir, path), false)
+                push!(files, name)
+            else
+                push!(dirs, name)
+            end
         end
-    end
 
-    function _it(chnl)
         if topdown
-            put!(chnl, (root, dirs, files))
+            push!(chnl, (root, dirs, files))
         end
         for dir in dirs
-            path = joinpath(root,dir)
-            if follow_symlinks || !islink(path)
-                for (root_l, dirs_l, files_l) in walkdir(path, topdown=topdown, follow_symlinks=follow_symlinks, onerror=onerror)
-                    put!(chnl, (root_l, dirs_l, files_l))
-                end
-            end
+            _walkdir(chnl, joinpath(root, dir))
         end
         if !topdown
-            put!(chnl, (root, dirs, files))
+            push!(chnl, (root, dirs, files))
         end
+        nothing
     end
-
-    return Channel(_it)
+    return Channel(chnl -> _walkdir(chnl, root))
 end
 
 function unlink(p::AbstractString)

base/filesystem.jl

@@ -219,26 +219,26 @@ const SEEK_END = Int32(2)
 
 function seek(f::File, n::Integer)
     ret = ccall(:jl_lseek, Int64, (OS_HANDLE, Int64, Int32), f.handle, n, SEEK_SET)
-    systemerror("seek", ret == -1)
+    ret == -1 && (@static Sys.iswindows() ? windowserror : systemerror)("seek")
     return f
 end
 
 function seekend(f::File)
     ret = ccall(:jl_lseek, Int64, (OS_HANDLE, Int64, Int32), f.handle, 0, SEEK_END)
-    systemerror("seekend", ret == -1)
+    ret == -1 && (@static Sys.iswindows() ? windowserror : systemerror)("seekend")
     return f
 end
 
 function skip(f::File, n::Integer)
     ret = ccall(:jl_lseek, Int64, (OS_HANDLE, Int64, Int32), f.handle, n, SEEK_CUR)
-    systemerror("skip", ret == -1)
+    ret == -1 && (@static Sys.iswindows() ? windowserror : systemerror)("skip")
     return f
 end
 
 function position(f::File)
     check_open(f)
     ret = ccall(:jl_lseek, Int64, (OS_HANDLE, Int64, Int32), f.handle, 0, SEEK_CUR)
-    systemerror("lseek", ret == -1)
+    ret == -1 && (@static Sys.iswindows() ? windowserror : systemerror)("lseek")
     return ret
 end
 

stdlib/REPL/src/REPLCompletions.jl

@@ -250,10 +250,10 @@ function complete_path(path::AbstractString, pos; use_envpath=false, shell_escap
                 filesinpath = readdir(pathdir)
             catch e
                 # Bash allows dirs in PATH that can't be read, so we should as well.
-                if isa(e, SystemError)
+                if isa(e, Base.IOError)
                     continue
                 else
-                    # We only handle SystemErrors here
+                    # We only handle IOError here
                     rethrow()
                 end
             end

test/file.jl

@@ -15,7 +15,7 @@ subdir = joinpath(dir, "adir")
 mkdir(subdir)
 subdir2 = joinpath(dir, "adir2")
 mkdir(subdir2)
-@test_throws Base.IOError mkdir(file)
+@test_throws Base._UVError("mkdir", Base.UV_EEXIST) mkdir(file)
 let err = nothing
     try
         mkdir(file)
@@ -445,9 +445,9 @@ close(f)
 
 rm(c_tmpdir, recursive=true)
 @test !isdir(c_tmpdir)
-@test_throws Base.IOError rm(c_tmpdir)
+@test_throws Base._UVError(Sys.iswindows() ? "chmod" : "unlink", Base.UV_ENOENT) rm(c_tmpdir)
 @test rm(c_tmpdir, force=true) === nothing
-@test_throws Base.IOError rm(c_tmpdir, recursive=true)
+@test_throws Base._UVError(Sys.iswindows() ? "chmod" : "unlink", Base.UV_ENOENT) rm(c_tmpdir, recursive=true)
 @test rm(c_tmpdir, force=true, recursive=true) === nothing
 
 if !Sys.iswindows()
@@ -462,8 +462,8 @@ if !Sys.iswindows()
         @test stat(file).gid == 0
         @test stat(file).uid == 0
     else
-        @test_throws Base.IOError chown(file, -2, -1)  # Non-root user cannot change ownership to another user
-        @test_throws Base.IOError chown(file, -1, -2)  # Non-root user cannot change group to a group they are not a member of (eg: nogroup)
+        @test_throws Base._UVError("chown", Base.UV_EPERM) chown(file, -2, -1)  # Non-root user cannot change ownership to another user
+        @test_throws Base._UVError("chown", Base.UV_EPERM) chown(file, -1, -2)  # Non-root user cannot change group to a group they are not a member of (eg: nogroup)
     end
 else
     # test that chown doesn't cause any errors for Windows
@@ -904,29 +904,31 @@ if !Sys.iswindows() || Sys.windows_version() >= Sys.WINDOWS_VISTA_VER
         for src in dirs
             for dst in dirs
                 # cptree
-                @test_throws ArgumentError Base.cptree(src,dst; force=true, follow_symlinks=false)
-                @test_throws ArgumentError Base.cptree(src,dst; force=true, follow_symlinks=true)
+                @test_throws ArgumentError Base.cptree(src, dst; force=true, follow_symlinks=false)
+                @test_throws ArgumentError Base.cptree(src, dst; force=true, follow_symlinks=true)
                 # cp
-                @test_throws ArgumentError cp(src,dst; force=true, follow_symlinks=false)
-                @test_throws ArgumentError cp(src,dst; force=true, follow_symlinks=true)
+                @test_throws ArgumentError cp(src, dst; force=true, follow_symlinks=false)
+                @test_throws ArgumentError cp(src, dst; force=true, follow_symlinks=true)
                 # mv
-                @test_throws ArgumentError mv(src,dst; force=true)
+                @test_throws ArgumentError mv(src, dst; force=true)
             end
         end
     end
     # None existing src
     mktempdir() do tmpdir
-        none_existing_src = joinpath(tmpdir, "none_existing_src")
+        nonexisting_src = joinpath(tmpdir, "nonexisting_src")
         dst = joinpath(tmpdir, "dst")
-        @test !ispath(none_existing_src)
+        @test !ispath(nonexisting_src)
         # cptree
-        @test_throws ArgumentError Base.cptree(none_existing_src,dst; force=true, follow_symlinks=false)
-        @test_throws ArgumentError Base.cptree(none_existing_src,dst; force=true, follow_symlinks=true)
+        @test_throws(ArgumentError("'$nonexisting_src' is not a directory. Use `cp(src, dst)`"),
+                     Base.cptree(nonexisting_src, dst; force=true, follow_symlinks=false))
+        @test_throws(ArgumentError("'$nonexisting_src' is not a directory. Use `cp(src, dst)`"),
+                     Base.cptree(nonexisting_src, dst; force=true, follow_symlinks=true))
         # cp
-        @test_throws Base.IOError cp(none_existing_src,dst; force=true, follow_symlinks=false)
-        @test_throws Base.IOError cp(none_existing_src,dst; force=true, follow_symlinks=true)
+        @test_throws Base._UVError("open", Base.UV_ENOENT) cp(nonexisting_src, dst; force=true, follow_symlinks=false)
+        @test_throws Base._UVError("open", Base.UV_ENOENT) cp(nonexisting_src, dst; force=true, follow_symlinks=true)
         # mv
-        @test_throws Base.IOError mv(none_existing_src,dst; force=true)
+        @test_throws Base._UVError("open", Base.UV_ENOENT) mv(nonexisting_src, dst; force=true)
     end
 end
 
@@ -1130,13 +1132,13 @@ if !Sys.iswindows()
         for src in files
             for dst in files
                 # cptree
-                @test_throws ArgumentError Base.cptree(src,dst; force=true, follow_symlinks=false)
-                @test_throws ArgumentError Base.cptree(src,dst; force=true, follow_symlinks=true)
+                @test_throws ArgumentError Base.cptree(src, dst; force=true, follow_symlinks=false)
+                @test_throws ArgumentError Base.cptree(src, dst; force=true, follow_symlinks=true)
                 # cp
-                @test_throws ArgumentError cp(src,dst; force=true, follow_symlinks=false)
-                @test_throws ArgumentError cp(src,dst; force=true, follow_symlinks=true)
+                @test_throws ArgumentError cp(src, dst; force=true, follow_symlinks=false)
+                @test_throws ArgumentError cp(src, dst; force=true, follow_symlinks=true)
                 # mv
-                @test_throws ArgumentError mv(src,dst; force=true)
+                @test_throws ArgumentError mv(src, dst; force=true)
             end
         end
     end
@@ -1298,7 +1300,8 @@ cd(dirwalk) do
     @test files == ["file1", "file2"]
 
     rm(joinpath("sub_dir1"), recursive=true)
-    @test_throws TaskFailedException take!(chnl_error) # throws an error because sub_dir1 do not exist
+    @test_throws(Base._UVError("readdir", Base.UV_ENOENT, "with ", repr(joinpath(".", "sub_dir1"))),
+                 take!(chnl_error)) # throws an error because sub_dir1 do not exist
 
     root, dirs, files = take!(chnl_noerror)
     @test root == "."
@@ -1313,9 +1316,12 @@ cd(dirwalk) do
     # Test that symlink loops don't cause errors
     if has_symlinks
         mkdir(joinpath(".", "sub_dir3"))
-        symlink("foo", joinpath(".", "sub_dir3", "foo"))
+        foo = joinpath(".", "sub_dir3", "foo")
+        symlink("foo", foo)
 
-        @test_throws Base.IOError walkdir(joinpath(".", "sub_dir3"); follow_symlinks=true)
+        let files = walkdir(joinpath(".", "sub_dir3"); follow_symlinks=true)
+            @test_throws Base._UVError("stat", Base.UV_ELOOP, "for file ", repr(foo)) take!(files)
+        end
         root, dirs, files = take!(walkdir(joinpath(".", "sub_dir3"); follow_symlinks=false))
         @test root == joinpath(".", "sub_dir3")
         @test dirs == []
@@ -1504,8 +1510,8 @@ end
                 rm(d, recursive=true)
                 @test !ispath(d)
                 @test isempty(readdir())
-                @test_throws SystemError readdir(d)
-                @test_throws Base.IOError readdir(join=true)
+                @test_throws Base._UVError("readdir", Base.UV_ENOENT, "with ", repr(d)) readdir(d)
+                @test_throws Base._UVError("cwd", Base.UV_ENOENT) readdir(join=true)
             end
         end
     end