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

WeakKeyDict is unsound #38727

Closed
Keno opened this issue Dec 5, 2020 · 12 comments · Fixed by #38802
Closed

WeakKeyDict is unsound #38727

Keno opened this issue Dec 5, 2020 · 12 comments · Fixed by #38802
Labels
bug Indicates an unexpected problem or unintended behavior GC Garbage collector

Comments

@Keno
Copy link
Member

Keno commented Dec 5, 2020

This is reduced from #38712 (comment). See that analysis for what happens in the original case. The short version is that there is a window of opportunity where the only reference to a value is weak, so finalizers can get scheduled, and then then a root can be re-established which causes much confusion. The below code illustrates the issue. This will return a foo(-1) because of this issue, which is illegal. It should either return foo(1) or missing.

mutable struct foo
	i::Int
end
const armed = Ref{Bool}(true)
@noinline fwdab(a, b) = invoke(Base.isequal, Tuple{Any, WeakRef}, a, b)
function Base.isequal(a::foo, b::WeakRef)
	# This GC.gc() here simulates a GC during compilation in the original issue
	armed[] && GC.gc()
        armed[] = false
	fwdab(a, b)
end
Base.isequal(a::WeakRef, b::foo) = isequal(b, a)
Base.:(==)(a::foo, b::foo) = a.i == b.i
Base.hash(a::foo, u::UInt) = Base.hash(a.i, u)

function make_foo(wkd, i)
	f = foo(i)
	function fin(f)
		f.i = -1
	end
	finalizer(fin, f)
	f
end

@noinline mkfoo(wkd) = wkd[make_foo(wkd, 1)] = nothing
function bar()
	wkd = WeakKeyDict{Any, Nothing}()
	mkfoo(wkd)
	armed[] = true
	z = getkey(wkd, foo(1), missing)
end
bar()
@Keno Keno added bug Indicates an unexpected problem or unintended behavior GC Garbage collector labels Dec 5, 2020
@Keno
Copy link
Member Author

Keno commented Dec 5, 2020

If trying the reproducer, make sure to try after 59aedd1. Without that, the reproducer needs to be written somewhat differently.

@StefanKarpinski
Copy link
Member

I can't reproduce on master (macOS).

@Keno
Copy link
Member Author

Keno commented Dec 6, 2020

Fails reliably for me on both linux and macOS master. What output of bar() do you see?

@StefanKarpinski
Copy link
Member

missing

@StefanKarpinski
Copy link
Member

Do I need to run it in a file?

@Keno
Copy link
Member Author

Keno commented Dec 6, 2020

No, I just paste it into the REPL and it fails. What commit are you on exactly?

@StefanKarpinski
Copy link
Member

Tried it loaded from a file, same result.

@StefanKarpinski
Copy link
Member

Maybe something specific about the macOS system you're testing on?

@Keno
Copy link
Member Author

Keno commented Dec 6, 2020

Shouldn't be, although it's somewhat sensitive to collection intervals. What happens if you runs bar twice and also what exact commit did you try?

@jmkuhn
Copy link
Contributor

jmkuhn commented Dec 6, 2020

I get foo(-1) on macOS with commit 65382c7.

@KristofferC
Copy link
Member

I also get foo(-1) on mac (every time).

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Dec 6, 2020

My bad — I was on a branch so when I did git pull and rebuilt Julia, I was not including 59aedd1. On master I get the same thing, so this seems like a robust reproducer after all. Sorry for the false alarm!

Keno added a commit that referenced this issue 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 issue Dec 10, 2020
The underlying issues was addressed with the change in WeakRef
semantics in #38180. However, we still want the test.
Closes #38727
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior GC Garbage collector
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants