Skip to content
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

channels: remove WeakRef from Condition #31673

Merged
merged 1 commit into from
Apr 11, 2019
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Apr 10, 2019

Using a WeakRef meant we might not actually bind the result.
If nobody was still holding a reference to put contents into the Condition,
we would simply garbage collect it, and then never need to close it.
Since that does not seem to be the intent,
instead move to just keeping a strong reference
(alternatively, we would have to switch to using stream_wait
with ref-counting, but that seems suboptimal for several reasons.).

fix #31507

@vtjnash vtjnash requested a review from amitmurthy April 10, 2019 17:00
@JeffBezanson
Copy link
Member

For those following along at home, and to see if I understand this, let me try to summarize. Consider the following task:

@async begin
    c = Channel(0)
    wait(c)
    fire_missiles()
end

Do the missiles ever get fired? No; nobody else has a reference to the channel, so wait will never return and the whole channel-and-task-bundle can just be GC'd. Now change it to:

@async begin
    c = Channel(0)
    bind(c, @async 0)
    try wait(c); catch; end
    fire_missiles()
end

bind says to close c when the @async 0 task exits. That would wake up the wait with an error, firing the missiles. But currently, we don't bother closing the channel if it has been GC'd (via a WeakRef), so whether the missiles fire is unpredictable. This PR changes it so that bind will always close the channel if its argument task exits.

In this case, "fire the missiles" means "run the rest of the test suite" :)

test/channels.jl Outdated Show resolved Hide resolved
Using a WeakRef meant we might not actually `bind` the result.
If nobody was still holding a reference to put contents into the Condition,
we would simply garbage collect it, and then never need to close it.
Since that does not seem to be the intent,
instead move to just keeping a strong reference
(alternatively, we would have to switch to using `stream_wait`
with ref-counting, but that seems suboptimal for several reasons.).

fix #31507
@JeffBezanson JeffBezanson merged commit 29f61cd into master Apr 11, 2019
@JeffBezanson JeffBezanson deleted the jn/channel-hang branch April 11, 2019 17:08
KristofferC pushed a commit that referenced this pull request Apr 15, 2019
Using a WeakRef meant we might not actually `bind` the result.
If nobody was still holding a reference to put contents into the Condition,
we would simply garbage collect it, and then never need to close it.
Since that does not seem to be the intent,
instead move to just keeping a strong reference
(alternatively, we would have to switch to using `stream_wait`
with ref-counting, but that seems suboptimal for several reasons.).

fix #31507

(cherry picked from commit 29f61cd)
@KristofferC KristofferC mentioned this pull request Apr 15, 2019
58 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

channels test hanging issue
4 participants