Skip to content

Commit 52da641

Browse files
vtjnashvtjnash
committed
fix PollingFileWatcher design and add workaround for a stat bug
What started as an innocent fix for a stat bug on Apple (#48667) turned into a full blown investigation into the design problems with the libuv backend for PollingFileWatcher, and writing my own implementation of it instead which could avoid those singled-threaded concurrency bugs.
1 parent 6e5e87b commit 52da641

File tree

7 files changed

+214
-166
lines changed

7 files changed

+214
-166
lines changed

‎base/libuv.jl

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,10 @@ for r in uv_req_types
2626
@eval const $(Symbol("_sizeof_", lowercase(string(r)))) = uv_sizeof_req($r)
2727
end
2828

29-
uv_handle_data(handle) = ccall(:jl_uv_handle_data, Ptr{Cvoid}, (Ptr{Cvoid},), handle)
30-
uv_req_data(handle) = ccall(:jl_uv_req_data, Ptr{Cvoid}, (Ptr{Cvoid},), handle)
31-
uv_req_set_data(req, data) = ccall(:jl_uv_req_set_data, Cvoid, (Ptr{Cvoid}, Any), req, data)
32-
uv_req_set_data(req, data::Ptr{Cvoid}) = ccall(:jl_uv_req_set_data, Cvoid, (Ptr{Cvoid}, Ptr{Cvoid}), req, data)
29+
uv_handle_data(handle) = ccall(:uv_handle_get_data, Ptr{Cvoid}, (Ptr{Cvoid},), handle)
30+
uv_req_data(handle) = ccall(:uv_req_get_data, Ptr{Cvoid}, (Ptr{Cvoid},), handle)
31+
uv_req_set_data(req, data) = ccall(:uv_req_set_data, Cvoid, (Ptr{Cvoid}, Any), req, data)
32+
uv_req_set_data(req, data::Ptr{Cvoid}) = ccall(:uv_handle_set_data, Cvoid, (Ptr{Cvoid}, Ptr{Cvoid}), req, data)
3333

3434
macro handle_as(hand, typ)
3535
return quote

‎base/reflection.jl

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -978,6 +978,7 @@ julia> structinfo(Base.Filesystem.StatStruct)
978978
(0x0000000000000050, :blocks, Int64)
979979
(0x0000000000000058, :mtime, Float64)
980980
(0x0000000000000060, :ctime, Float64)
981+
(0x0000000000000068, :ioerrno, Int32)
981982
```
982983
"""
983984
fieldoffset(x::DataType, idx::Integer) = (@_foldable_meta; ccall(:jl_get_field_offset, Csize_t, (Any, Cint), x, idx))

‎base/stat.jl

Lines changed: 57 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ struct StatStruct
6363
blocks :: Int64
6464
mtime :: Float64
6565
ctime :: Float64
66+
ioerrno :: Int32
6667
end
6768

6869
@eval function Base.:(==)(x::StatStruct, y::StatStruct) # do not include `desc` in equality or hash
@@ -80,22 +81,23 @@ end
8081
end)
8182
end
8283

83-
StatStruct() = StatStruct("", 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0)
84-
StatStruct(buf::Union{Vector{UInt8},Ptr{UInt8}}) = StatStruct("", buf)
85-
StatStruct(desc::Union{AbstractString, OS_HANDLE}, buf::Union{Vector{UInt8},Ptr{UInt8}}) = StatStruct(
84+
StatStruct() = StatStruct("", 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, Base.UV_ENOENT)
85+
StatStruct(buf::Union{Memory{UInt8},Vector{UInt8},Ptr{UInt8}}, ioerrno::Int32) = StatStruct("", buf, ioerrno)
86+
StatStruct(desc::Union{AbstractString, OS_HANDLE}, buf::Union{Memory{UInt8},Vector{UInt8},Ptr{UInt8}}, ioerrno::Int32) = StatStruct(
8687
desc isa OS_HANDLE ? desc : String(desc),
87-
ccall(:jl_stat_dev, UInt32, (Ptr{UInt8},), buf),
88-
ccall(:jl_stat_ino, UInt32, (Ptr{UInt8},), buf),
89-
ccall(:jl_stat_mode, UInt32, (Ptr{UInt8},), buf),
90-
ccall(:jl_stat_nlink, UInt32, (Ptr{UInt8},), buf),
91-
ccall(:jl_stat_uid, UInt32, (Ptr{UInt8},), buf),
92-
ccall(:jl_stat_gid, UInt32, (Ptr{UInt8},), buf),
93-
ccall(:jl_stat_rdev, UInt32, (Ptr{UInt8},), buf),
94-
ccall(:jl_stat_size, UInt64, (Ptr{UInt8},), buf),
95-
ccall(:jl_stat_blksize, UInt64, (Ptr{UInt8},), buf),
96-
ccall(:jl_stat_blocks, UInt64, (Ptr{UInt8},), buf),
97-
ccall(:jl_stat_mtime, Float64, (Ptr{UInt8},), buf),
98-
ccall(:jl_stat_ctime, Float64, (Ptr{UInt8},), buf),
88+
ioerrno != 0 ? zero(UInt32) : ccall(:jl_stat_dev, UInt32, (Ptr{UInt8},), buf),
89+
ioerrno != 0 ? zero(UInt32) : ccall(:jl_stat_ino, UInt32, (Ptr{UInt8},), buf),
90+
ioerrno != 0 ? zero(UInt32) : ccall(:jl_stat_mode, UInt32, (Ptr{UInt8},), buf),
91+
ioerrno != 0 ? zero(UInt32) : ccall(:jl_stat_nlink, UInt32, (Ptr{UInt8},), buf),
92+
ioerrno != 0 ? zero(UInt32) : ccall(:jl_stat_uid, UInt32, (Ptr{UInt8},), buf),
93+
ioerrno != 0 ? zero(UInt32) : ccall(:jl_stat_gid, UInt32, (Ptr{UInt8},), buf),
94+
ioerrno != 0 ? zero(UInt32) : ccall(:jl_stat_rdev, UInt32, (Ptr{UInt8},), buf),
95+
ioerrno != 0 ? zero(UInt64) : ccall(:jl_stat_size, UInt64, (Ptr{UInt8},), buf),
96+
ioerrno != 0 ? zero(UInt64) : ccall(:jl_stat_blksize, UInt64, (Ptr{UInt8},), buf),
97+
ioerrno != 0 ? zero(UInt64) : ccall(:jl_stat_blocks, UInt64, (Ptr{UInt8},), buf),
98+
ioerrno != 0 ? zero(Float64) : ccall(:jl_stat_mtime, Float64, (Ptr{UInt8},), buf),
99+
ioerrno != 0 ? zero(Float64) : ccall(:jl_stat_ctime, Float64, (Ptr{UInt8},), buf),
100+
ioerrno
99101
)
100102

101103
function iso_datetime_with_relative(t, tnow)
@@ -130,35 +132,41 @@ end
130132
function show_statstruct(io::IO, st::StatStruct, oneline::Bool)
131133
print(io, oneline ? "StatStruct(" : "StatStruct for ")
132134
show(io, st.desc)
133-
oneline || print(io, "\n ")
134-
print(io, " size: ", st.size, " bytes")
135-
oneline || print(io, "\n")
136-
print(io, " device: ", st.device)
137-
oneline || print(io, "\n ")
138-
print(io, " inode: ", st.inode)
139-
oneline || print(io, "\n ")
140-
print(io, " mode: 0o", string(filemode(st), base = 8, pad = 6), " (", filemode_string(st), ")")
141-
oneline || print(io, "\n ")
142-
print(io, " nlink: ", st.nlink)
143-
oneline || print(io, "\n ")
144-
print(io, " uid: $(st.uid)")
145-
username = getusername(st.uid)
146-
username === nothing || print(io, " (", username, ")")
147-
oneline || print(io, "\n ")
148-
print(io, " gid: ", st.gid)
149-
groupname = getgroupname(st.gid)
150-
groupname === nothing || print(io, " (", groupname, ")")
151-
oneline || print(io, "\n ")
152-
print(io, " rdev: ", st.rdev)
153-
oneline || print(io, "\n ")
154-
print(io, " blksz: ", st.blksize)
155-
oneline || print(io, "\n")
156-
print(io, " blocks: ", st.blocks)
157-
tnow = round(UInt, time())
158-
oneline || print(io, "\n ")
159-
print(io, " mtime: ", iso_datetime_with_relative(st.mtime, tnow))
160-
oneline || print(io, "\n ")
161-
print(io, " ctime: ", iso_datetime_with_relative(st.ctime, tnow))
135+
code = st.ioerrno
136+
if code != 0
137+
print(io, oneline ? " " : "\n ")
138+
print(io, Base.uverrorname(code), ": ", Base.struverror(code))
139+
else
140+
oneline || print(io, "\n ")
141+
print(io, " size: ", st.size, " bytes")
142+
oneline || print(io, "\n")
143+
print(io, " device: ", st.device)
144+
oneline || print(io, "\n ")
145+
print(io, " inode: ", st.inode)
146+
oneline || print(io, "\n ")
147+
print(io, " mode: 0o", string(filemode(st), base = 8, pad = 6), " (", filemode_string(st), ")")
148+
oneline || print(io, "\n ")
149+
print(io, " nlink: ", st.nlink)
150+
oneline || print(io, "\n ")
151+
print(io, " uid: $(st.uid)")
152+
username = getusername(st.uid)
153+
username === nothing || print(io, " (", username, ")")
154+
oneline || print(io, "\n ")
155+
print(io, " gid: ", st.gid)
156+
groupname = getgroupname(st.gid)
157+
groupname === nothing || print(io, " (", groupname, ")")
158+
oneline || print(io, "\n ")
159+
print(io, " rdev: ", st.rdev)
160+
oneline || print(io, "\n ")
161+
print(io, " blksz: ", st.blksize)
162+
oneline || print(io, "\n")
163+
print(io, " blocks: ", st.blocks)
164+
tnow = round(UInt, time())
165+
oneline || print(io, "\n ")
166+
print(io, " mtime: ", iso_datetime_with_relative(st.mtime, tnow))
167+
oneline || print(io, "\n ")
168+
print(io, " ctime: ", iso_datetime_with_relative(st.ctime, tnow))
169+
end
162170
oneline && print(io, ")")
163171
return nothing
164172
end
@@ -168,18 +176,13 @@ show(io::IO, ::MIME"text/plain", st::StatStruct) = show_statstruct(io, st, false
168176

169177
# stat & lstat functions
170178

179+
checkstat(s::StatStruct) = Int(s.ioerrno) in (0, Base.UV_ENOENT, Base.UV_ENOTDIR, Base.UV_EINVAL) ? s : uv_error(string("stat(", repr(s.desc), ")"), s.ioerrno)
180+
171181
macro stat_call(sym, arg1type, arg)
172182
return quote
173-
stat_buf = zeros(UInt8, Int(ccall(:jl_sizeof_stat, Int32, ())))
183+
stat_buf = fill!(Memory{UInt8}(undef, Int(ccall(:jl_sizeof_stat, Int32, ()))), 0x00)
174184
r = ccall($(Expr(:quote, sym)), Int32, ($(esc(arg1type)), Ptr{UInt8}), $(esc(arg)), stat_buf)
175-
if !(r in (0, Base.UV_ENOENT, Base.UV_ENOTDIR, Base.UV_EINVAL))
176-
uv_error(string("stat(", repr($(esc(arg))), ")"), r)
177-
end
178-
st = StatStruct($(esc(arg)), stat_buf)
179-
if ispath(st) != (r == 0)
180-
error("stat returned zero type for a valid path")
181-
end
182-
return st
185+
return checkstat(StatStruct($(esc(arg)), stat_buf, r))
183186
end
184187
end
185188

@@ -334,7 +337,7 @@ Return `true` if a valid filesystem entity exists at `path`,
334337
otherwise returns `false`.
335338
This is the generalization of [`isfile`](@ref), [`isdir`](@ref) etc.
336339
"""
337-
ispath(st::StatStruct) = filemode(st) & 0xf000 != 0x0000
340+
ispath(st::StatStruct) = st.ioerrno == 0
338341
function ispath(path::String)
339342
# We use `access()` and `F_OK` to determine if a given path exists. `F_OK` comes from `unistd.h`.
340343
F_OK = 0x00

‎src/sys.c

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,6 @@ JL_DLLEXPORT int32_t jl_nb_available(ios_t *s)
102102

103103
// --- dir/file stuff ---
104104

105-
JL_DLLEXPORT int jl_sizeof_uv_fs_t(void) { return sizeof(uv_fs_t); }
106105
JL_DLLEXPORT char *jl_uv_fs_t_ptr(uv_fs_t *req) { return (char*)req->ptr; }
107106
JL_DLLEXPORT char *jl_uv_fs_t_path(uv_fs_t *req) { return (char*)req->path; }
108107

0 commit comments

Comments
 (0)