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

fix numerous issues with WeakKeyDict #38180

Merged
merged 3 commits into from
Dec 8, 2020
Merged

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Oct 26, 2020

Delay cleanup of WeakKeyDict items until the next insertion. (And fix
get!, since previously usage of it would have added keys without
finalizers to the dict.)

The most significant change here is that I'm changing the way WeakKey works: now instead of the GC clearing the internal reference after the last instance of it disappears from the system (after running finalizers), I'm instead clearing it prior to (happens-before) scheduling any finalization. This is significant because—while this breaks the old implementation of WeakKeyDict—it is a necessary property for precise detection of whether the object is still strongly alive after recovering a reference to it out of a WeakKey object. This happens to be how other languages do it (https://docs.oracle.com/javase/8/docs/api/java/lang/ref/WeakReference.html, https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/weak-references), so I feel reassured this is the right change.

This also means we could also now drop the internal lock that was need to make this thread-safe (not implemented yet). Thoughts?

Fixes #26939

@JeffBezanson
Copy link
Member

I like the general idea but there are still some issues. For example I was able to look up nothing:

julia> d = WeakKeyDict{Any,Any}();

julia> d[[1]]=1
julia> d[[2]]=1
julia> d[[3]]=1
julia> d[[4]]=1
julia> d[[5]]=1

julia> GC.gc()

julia> d[nothing]
1

julia> d
WeakKeyDict{Any, Any} with 5 entries:

julia> 

length is a tough one. It should probably also call _cleanup_locked. Of course it's still hard to use correctly since the true length can change at any time. But we could at least have the property that the length is correct if no GC happens (haha) during the process of getting the length and then iterating.

@vtjnash
Copy link
Member Author

vtjnash commented Oct 26, 2020

Yeah, I'm not quite sure how to deal with that. I'm considering just defining summary to print "at most", so it's less odd (and define it as SizeUnknown for collect). I don't really want to make length O(n) or mutating, but it is non-trivial then to do better than this.

@Keno
Copy link
Member

Keno commented Dec 6, 2020

Overall direction looks right to me.

Delay cleanup of WeakKeyDict items until the next insertion. And fix
`get!`, since previously usage of it would have added keys without
finalizers to the dict.

Fixes #26939
base/weakkeydict.jl Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Member

It still seems worthwhile to me for length to call _cleanup_locked.

@JeffBezanson JeffBezanson changed the title RFC: fix numerous issues with WeakKeyDict fix numerous issues with WeakKeyDict Dec 8, 2020
@JeffBezanson JeffBezanson merged commit 624e734 into master Dec 8, 2020
@JeffBezanson JeffBezanson deleted the jn/weakkeydict-safer branch December 8, 2020 22:09
Keno added a commit that referenced this pull request Dec 9, 2020
The underlying issues was addressed with the change in WeakRef
semantics in #38180. However, we still want the test.
Closes #38727
Keno added a commit that referenced this pull request Dec 10, 2020
The underlying issues was addressed with the change in WeakRef
semantics in #38180. However, we still want the test.
Closes #38727
@giordano giordano mentioned this pull request Feb 24, 2021
52 tasks
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
Delay cleanup of WeakKeyDict items until the next insertion. And fix
`get!`, since previously usage of it would have added keys without
finalizers to the dict.

Fixes JuliaLang#26939
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
The underlying issues was addressed with the change in WeakRef
semantics in JuliaLang#38180. However, we still want the test.
Closes JuliaLang#38727
@tthsqe12 tthsqe12 mentioned this pull request Oct 2, 2021
vtjnash pushed a commit that referenced this pull request Apr 4, 2022
Thanks to #38180, the removed code seems to be dead because no finalizers should be used anymore to modify dictionaries (it was dangerous). Furthermore, it may help users to detect illegal concurrent writes, since it doesn't recurse and have new error message. There should be no, or even a positive, performance effect.
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.

BigFloat stored in WeakKeyDicts cause segfaults when garbage collected
3 participants