-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 missing GC.add_finalizer throughout the stdlib #5367
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this for a quick fix but I'd like to reduce the number of times we use allocate
/initialize
explicitly instead of just new
.
|
||
GC.collect | ||
|
||
State.count(key).should be > 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not exactly 10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_finalizes
is based on weak_ref_spec
which uses exactly the same expectations, so I was trying to follow the same style. At least one object (the last one) in the series is not freed (because it's referenced by obj
). Secondarily, some objects require multiple calls to GC.collect
to be collected. So I guess it's safe to assume if at least one object of given type is collected, the entire type is collectable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow up: the last object in the iteration is not released because it is still referenced by the obj
variable outside the loop.
Adding specs is nice, but they depend on the garbage collector collecting expected objects at the expected time, which... may not happen (e.g. parallel collection). I believe we can safely skip them. The only places calling |
Fixes #5362