Skip to content

Commit

Permalink
filesystem: fix some error handling (#36856)
Browse files Browse the repository at this point in the history 
walkdir: fix thrown error location and types and performance
readdir: fix thrown error code on some OS
seek: fix thrown error code on Windows
  • Loading branch information
@vtjnash
vtjnash authored Aug 7, 2020
1 parent 7c0cb30 commit dffc889
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 70 deletions.
70 changes: 33 additions & 37 deletions base/file.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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[]
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
8 changes: 4 additions & 4 deletions base/filesystem.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")

This comment has been minimized.

Sign in to view

Copy link
@StefanKarpinski

StefanKarpinski Aug 10, 2020

Member

Wouldn't it make more sense to unify windowserror and systemerror?
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

Expand Down
4 changes: 2 additions & 2 deletions stdlib/REPL/src/REPLCompletions.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
60 changes: 33 additions & 27 deletions test/file.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand All @@ -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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 == "."
Expand All @@ -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 == []
Expand Down Expand Up @@ -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
Expand Down

6 comments on commit dffc889

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your package evaluation job has completed - possible issues were detected. A full report can be found here. cc @maleadt

Please sign in to comment.