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.

Sign up for GitHub

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

Jump to bottom

IO: tie lifetime of handle field to container #43218

Merged
merged 1 commit into from
Jan 24, 2022
Merged

IO: tie lifetime of handle field to container #43218

merged 1 commit into from
Jan 24, 2022

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Nov 25, 2021

Rather than freeing this memory as soon as possible, ensure that the
lifetime of the handle is always >= the container object. This lets us
examine some (limited) aspects of the handle without holding a lock.

And we also examine and fix numerous other thread-safety and
synchronization bugs too.

@vtjnash
IO: tie lifetime of handle field to container
82741d6 
Rather than freeing this memory as soon as possible, ensure that the
lifetime of the handle is always >= the container object. This lets us
examine some (limited) aspects of the handle without holding a lock.

And we also examine and fix numerous other thread-safety and
synchronization bugs too.
@vtjnash vtjnash requested a review from tkf November 25, 2021 02:49
tkf
tkf reviewed Nov 25, 2021
View reviewed changes
Copy link
Member

@tkf tkf left a comment

Choose a reason for hiding this comment

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

I don't fully understand this part of code so all I can do is some nitpicks

src/jl_uv.c Show resolved Hide resolved
base/stream.jl Show resolved Hide resolved
@vtjnash vtjnash merged commit 5cd31b5 into master Jan 24, 2022
@vtjnash vtjnash deleted the jn/io-handle-life branch January 24, 2022 23:17
@Keno
Copy link
Member

Keno commented Jan 25, 2022

This appears to have broken CI.

Keno added a commit that referenced this pull request Jan 25, 2022
@Keno
Revert "IO: tie lifetime of handle field to container (#43218)"
02a7af4 
This reverts commit 5cd31b5.
@Keno Keno mentioned this pull request Jan 25, 2022
Merged
Keno added a commit that referenced this pull request Jan 25, 2022
@Keno
Revert "IO: tie lifetime of handle field to container (#43218)" (#43924)
a400a24 
This reverts commit 5cd31b5.
@Keno Keno restored the jn/io-handle-life branch January 26, 2022 04:02
@Keno Keno deleted the jn/io-handle-life branch January 26, 2022 04:03
@Keno
Copy link
Member

Keno commented Jan 26, 2022

I tried re-opening the PR but that doesn't appear to work. Probably best to do a new PR to revert the revert with whatever fixes are required to not break CI.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jan 26, 2022 •
edited
Loading

Yeah, GitHub doesn't want to have 2 merges from a single PR. I was working on figuring this out locally, and will push a new PR when I have solved it.

vtjnash added a commit that referenced this pull request Feb 7, 2022
@vtjnash
Reland "IO: tie lifetime of handle field to container (#43218)"
02a2e26 
Reverts a400a24 (#43924).

Fix lifetime issues with the original attempt. We do not need to prevent
it from being GC-finalized: only to make sure it does not conflict with
a concurrent uv_close callback.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@vtjnash @LilithHafner
IO: tie lifetime of handle field to container (JuliaLang#43218)
729ae51 
Rather than freeing this memory as soon as possible, ensure that the
lifetime of the handle is always >= the container object. This lets us
examine some (limited) aspects of the handle without holding a lock.

And we also examine and fix numerous other thread-safety and
synchronization bugs too.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@Keno @LilithHafner
Revert "IO: tie lifetime of handle field to container (JuliaLang#43218)…
3e4d1f7 
…" (JuliaLang#43924)

This reverts commit 5cd31b5.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@vtjnash @LilithHafner
IO: tie lifetime of handle field to container (JuliaLang#43218)
35683e3 
Rather than freeing this memory as soon as possible, ensure that the
lifetime of the handle is always >= the container object. This lets us
examine some (limited) aspects of the handle without holding a lock.

And we also examine and fix numerous other thread-safety and
synchronization bugs too.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@Keno @LilithHafner
Revert "IO: tie lifetime of handle field to container (JuliaLang#43218)…
08bcfaf 
…" (JuliaLang#43924)

This reverts commit 5cd31b5.
vtjnash added a commit that referenced this pull request Apr 4, 2022
@vtjnash
Reland "IO: tie lifetime of handle field to container (#43218)"
745d65b 
Reverts a400a24 (#43924).

Fix lifetime issues with the original attempt. We do not need to prevent
it from being GC-finalized: only to make sure it does not conflict with
a concurrent uv_close callback.
vtjnash added a commit that referenced this pull request Apr 6, 2022
@vtjnash
Reland "IO: tie lifetime of handle field to container (#43218)"
f2644f8 
Reverts a400a24 (#43924).

Fix lifetime issues with the original attempt. We do not need to prevent
it from being GC-finalized: only to make sure it does not conflict with
a concurrent uv_close callback.
vtjnash added a commit that referenced this pull request Apr 6, 2022
@vtjnash
Reland "IO: tie lifetime of handle field to container (#43218)"
4f5cf90 
Reverts a400a24 (#43924).

Fix lifetime issues with the original attempt. We do not need to prevent
it from being GC-finalized: only to make sure it does not conflict with
a concurrent uv_close callback.
vtjnash added a commit that referenced this pull request Apr 12, 2022
@vtjnash
Reland "IO: tie lifetime of handle field to container (#43218)"
1b8f074 
Reverts a400a24 (#43924).

Fix lifetime issues with the original attempt. We do not need to prevent
it from being GC-finalized: only to make sure it does not conflict with
a concurrent uv_close callback.
@vtjnash vtjnash mentioned this pull request Apr 12, 2022
Merged
vtjnash added a commit that referenced this pull request Apr 12, 2022
@vtjnash
Reland "IO: tie lifetime of handle field to container (#43218)"
6573350 
Reverts a400a24 (#43924).

Fix lifetime issues with the original attempt. We do not need to prevent
it from being GC-finalized: only to make sure it does not conflict with
a concurrent uv_close callback.
vtjnash added a commit that referenced this pull request Apr 13, 2022
@vtjnash
Reland "IO: tie lifetime of handle field to container (#43218)"
f3fe012 
Reverts a400a24 (#43924).

Fix lifetime issues with the original attempt. We do not need to prevent
it from being GC-finalized: only to make sure it does not conflict with
a concurrent uv_close callback.
vtjnash added a commit that referenced this pull request Apr 14, 2022
@vtjnash
Reland "IO: tie lifetime of handle field to container (#43218)" (#44883)
72cd1c4 
Reverts a400a24 (#43924).

Fix lifetime issues with the original attempt. We do not need to prevent
it from being GC-finalized: only to make sure it does not conflict with
a concurrent uv_close callback.
DilumAluthge added a commit that referenced this pull request Apr 21, 2022
@DilumAluthge
Revert "Reland "IO: tie lifetime of handle field to container (#43218)…
95e5fd9 
…" (#44883)"

This reverts commit 72cd1c4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkf tkf tkf left review comments

@JeffBezanson JeffBezanson Awaiting requested review from JeffBezanson

@Keno Keno Awaiting requested review from Keno

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vtjnash @Keno @tkf