Skip to content

Clarify documentation around close, flush, and isopen #58020

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions base/asyncevent.jl
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ Create a async condition that wakes up tasks waiting for it
(by calling [`wait`](@ref) on the object)
when notified from C by a call to `uv_async_send`.
Waiting tasks are woken with an error when the object is closed (by [`close`](@ref)).
Use [`isopen`](@ref) to check whether it is still active.
Use [`isopen`](@ref) to check whether it is still active. A closed condition will
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this would be better? Yes, the function is called isopen but the text says "active", not "open?

Suggested change
Use [`isopen`](@ref) to check whether it is still active. A closed condition will
Use [`isopen`](@ref) to check whether it is still active. An inactive condition will

Copy link
Member

Choose a reason for hiding this comment

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

OK I see elsewhere the text already talks about "closed". I find this use of two different terms meaning the same thing adds a layer of potential confusion. So perhaps like this:

Suggested change
Use [`isopen`](@ref) to check whether it is still active. A closed condition will
Use [`isopen`](@ref) to check whether it is still active. A closed condition is inactive and will

not wake up tasks.

This provides an implicit acquire & release memory ordering between the sending and waiting threads.
"""
Expand Down Expand Up @@ -74,7 +75,8 @@ Create a timer that wakes up tasks waiting for it (by calling [`wait`](@ref) on
Waiting tasks are woken after an initial delay of at least `delay` seconds, and then repeating after
at least `interval` seconds again elapse. If `interval` is equal to `0`, the timer is only triggered
once. When the timer is closed (by [`close`](@ref)) waiting tasks are woken with an error. Use
[`isopen`](@ref) to check whether a timer is still active. Use `t.timeout` and `t.interval` to read
[`isopen`](@ref) to check whether a timer is still active. A closed timer will not fire.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[`isopen`](@ref) to check whether a timer is still active. A closed timer will not fire.
[`isopen`](@ref) to check whether a timer is still active. An inactive timer will not fire.

Use `t.timeout` and `t.interval` to read
the setup conditions of a `Timer` `t`.

```julia-repl
Expand Down Expand Up @@ -206,6 +208,14 @@ end

isopen(t::Union{Timer, AsyncCondition}) = @atomic :acquire t.isopen

"""
close(t::Union{Timer, AsyncCondition})

Close an object `t`. Once a timer or condition is closed, it will not produce
Copy link
Member

Choose a reason for hiding this comment

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

Continuing from above, how about

Suggested change
Close an object `t`. Once a timer or condition is closed, it will not produce
Close an object `t` and thus mark it as inactive. Once a timer or condition is inactive, it will not produce

a new event.

See also: [`isopen`](@ref)
"""
function close(t::Union{Timer, AsyncCondition})
t.handle == C_NULL && !t.isopen && return # short-circuit path, :monotonic
iolock_begin()
Expand Down
36 changes: 26 additions & 10 deletions base/io.jl
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,9 @@ buffer_writes(x::IO, bufsize=SZ_UNBUFFERED_IO) = x
"""
isopen(object)::Bool

Determine whether an object - such as a stream or timer
-- is not yet closed. Once an object is closed, it will never produce a new event.
However, since a closed stream may still have data to read in its buffer,
use [`eof`](@ref) to check for the ability to read data.
Use the `FileWatching` package to be notified when a stream might be writable or readable.
Determine whether an object, such as an IO or timer, is not yet closed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Determine whether an object, such as an IO or timer, is not yet closed.
Determine whether an object, such as an IO or timer, is still open and hence active.


See also: [`close`](@ref)

# Examples
```jldoctest
Expand All @@ -63,11 +61,27 @@ false
function isopen end

"""
close(stream)
close(io::IO)

Close `io`. Performs a [`flush`](@ref) first.

Closing an IO signals that its underlying resources (OS handle, network
connections, etc) should be destroyed.
A closed IO is in an undefined state and should not be written to or read from.
When attempting to do so, the IO may throw an exception, continue to behave
normally, or read/write zero bytes, depending on the implementation.
However, implementations should make sure that reading to or writing from a
closed IO does not cause undefined behaviour.

Close an I/O stream. Performs a [`flush`](@ref) first.
This function is generically defined to only `flush` the io. That allows
wrapping IOs to close its underlying IO.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
wrapping IOs to close its underlying IO.
wrapping IOs to close their underlying IOs.


See also: [`isopen`](@ref)
"""
function close end
function close(io::IO)
flush(io)
nothing
end
Comment on lines +81 to +84
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a change in behavior and thus goes beyond what the title of this PR suggests? I feel a bit uncomfortable with this -- I'd immediately agree to merge the docstring changes, but I don't feel qualified to judge whether this change is OK.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. The rest seems good


"""
closewrite(stream)
Expand Down Expand Up @@ -97,9 +111,11 @@ julia> read(io, String)
function closewrite end

"""
flush(stream)
flush(io::IO)

Commit all currently buffered writes to the given stream.
Commit all currently buffered writes to the given io.
This has a default implementation `flush(::IO) = nothing`, so may be called
in generic IO code.
"""
function flush end

Expand Down
10 changes: 10 additions & 0 deletions base/iostream.jl
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,16 @@ fd(s::IOStream) = RawFD(ccall(:jl_ios_fd, Clong, (Ptr{Cvoid},), s.ios))

stat(s::IOStream) = stat(fd(s))

"""
isopen(s::IOStream)

Check if the stream is not yet closed.

A closed `IOStream` may still have data to read in its buffer,
use [`eof`](@ref) to check for the ability to read data.

Use the `FileWatching` package to be notified when a file might be writable or readable.
"""
isopen(s::IOStream) = ccall(:ios_isopen, Cint, (Ptr{Cvoid},), s.ios) != 0

function close(s::IOStream)
Expand Down