Skip to content

Channel iteration uses inconsistent termination criteria #50298

Open

Description

Since #45966, Channel iteration uses inconsistent termination criteria. Consider the implementation:

julia/base/channels.jl

Lines 591 to 605 in 269d350

function iterate(c::Channel, state=nothing)
if isopen(c) || isready(c)
try
return (take!(c), nothing)
catch e
if isa(e, InvalidStateException) && e.state === :closed
return nothing
else
rethrow()
end
end
else
return nothing
end
end

In the absence of a race condition, iteration terminates when the channel is closed and empty as determined by the conditional. However, if you end up in the catch block, the criterion for deciding whether to terminate iteration (i.e., you're just here because of a race condition while closing the channel) or rethrow (there's an actual problem) is based on the type and value of the exception, not on the state of the channel.

The two criteria are consistent if the channel was closed using close(c), but not if you attach a custom exception as in close(c, CustomException()). I can see three different ways to achieve consistency without reverting to the old, slower design.

  • Alternative 1: Deprecate close(c::Channel, excp::Exception) in favor of close(c::Channel, msg::String), which sets c.excp = InvalidStateException(msg, :closed). That way, take!(c) on a closed channel always raises an error that satisfies the termination criterion.

  • Alternative 2: Decide that closed channel iteration should always terminate normally, regardless of c.excp. To achieve this, relax the catch block criterion to only verify that the channel is closed and empty and that the exception is indeed c.excp:

function iterate(c::Channel, state=nothing)
    if isopen(c) || isready(c)
        try
            return (take!(c), nothing)
        catch e
            if !(isopen(c) || isready(c)) && (c.excp === e || (c.excp === nothing && isequal(e, closed_exception())))
                # c.excp === nothing included for consistency with check_channel_state(c)
                return nothing
            else
                rethrow()
            end
        end
    else
        return nothing
    end
end
  • Alternative 3: Stick to the behavior from before Add channel state checks to reduce exceptions #45966, such that closed channel iteration only terminates normally if take!(c) would throw InvalidStateException(msg, :closed). To enforce this, perform an extra inspection in the else block:
function iterate(c::Channel, state=nothing)
    if isopen(c) || isready(c)
        try
            return (take!(c), nothing)
        catch e
            if isa(e, InvalidStateException) && e.state === :closed
                return nothing
            else
                rethrow()
            end
        end
    else
        # adapted from check_channel_state(c); this is what take!(c) would do before throwing
        (@atomic :acquire c.state) === :open && concurrency_violation()
        e = c.excp
        # e === nothing included for consistency with check_channel_state(c)
        if (e === nothing) || (isa(e, InvalidStateException) && e.state === :closed)
            return nothing
        else
            throw(e)
        end
    end
end

Happy to make a PR if there's consensus on which solution to go with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    bugIndicates an unexpected problem or unintended behaviorparallelismParallel or distributed computation

    Type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions