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

Fix missing GC.add_finalizer throughout the stdlib #5367

Merged
merged 1 commit into from Jan 2, 2018
Merged

Fix missing GC.add_finalizer throughout the stdlib #5367

merged 1 commit into from Jan 2, 2018

Conversation

alexbatalov
Copy link
Contributor

@alexbatalov alexbatalov commented Dec 8, 2017 •
edited

Fixes #5362

@alexbatalov alexbatalov changed the title Fix missing GC.add_finalizer throughout the stdlib [5362] Fix missing GC.add_finalizer throughout the stdlib Dec 8, 2017
RX14
RX14 approved these changes Dec 8, 2017
View reviewed changes
Copy link
Contributor

@RX14 RX14 left a 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.

lbguilherme
lbguilherme reviewed Dec 8, 2017
View reviewed changes
spec/support/finalize.cr

GC.collect

State.count(key).should be > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not exactly 10?

Copy link
Contributor Author

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.

Copy link
Member

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.

@ysbaddaden
Copy link
Contributor

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 allocate in the corelib should be Reference#dup and Object#clone. OpenSSL::SSL::Context should be fixed to use factories instead (see ysbaddaden@4568b16 for example). I'm not sure about YAML.mapping, from a quick look it seems unavoidable.

@RX14 RX14 added this to the Next milestone Jan 2, 2018
@RX14 RX14 merged commit 0e48aac into crystal-lang:master Jan 2, 2018
lukeasrodgers pushed a commit to lukeasrodgers/crystal that referenced this pull request Jan 7, 2018
@alexbatalov @lukeasrodgers
Fix missing GC.add_finalizer throughout the stdlib (crystal-lang#5367)
f9c125b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcardiff bcardiff bcardiff left review comments

@lbguilherme lbguilherme lbguilherme left review comments

@asterite asterite asterite approved these changes

@RX14 RX14 RX14 approved these changes

Assignees
No one assigned
Labels
kind:bug topic:stdlib
Projects
None yet
Milestone
0.25.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@alexbatalov @ysbaddaden @asterite @bcardiff @lbguilherme @RX14