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

GC.enable: raise if GC is not disabled #4945

Merged
merged 2 commits into from Sep 15, 2017
Merged

GC.enable: raise if GC is not disabled #4945

merged 2 commits into from Sep 15, 2017

Conversation

asterite
Copy link
Member

@asterite asterite commented Sep 10, 2017

Fixes #4721

@oprypin
Copy link
Member

@oprypin oprypin commented Sep 10, 2017

@asterite
Copy link
Member Author

@asterite asterite commented Sep 10, 2017

@oprypin There's no assertion there, as far as I can see. The idea is that you can invoke disable many times, and a counter is added, and then you need to invoke the same amount of enable to finally enable it. That's because you might want to do:

GC.disable
run_something
GC.enable

and run_something might end up also disabling and enabling the GC, and without a counter that's impossible to do.

src/gc/boehm.cr Outdated
@@ -85,6 +86,10 @@ module GC
end

def self.enable
unless LibGC.is_disabled != 0
raise "GC.enable: GC is not disabled"
Copy link
Contributor

@RX14 RX14 Sep 10, 2017

Choose a reason for hiding this comment

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

We should already know we're in GC.enable from the stacktrace. We don't prefix with the method name in other parts of the stdlib at least.

Copy link
Member Author

@asterite asterite Sep 10, 2017

Choose a reason for hiding this comment

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

The stack trace I get is:

GC is not disabled (Exception)
0x108ef5695: *CallStack::unwind:Array(Pointer(Void)) at ??
0x108ef5631: *CallStack#initialize:Array(Pointer(Void)) at ??
0x108ef5608: *CallStack::new:CallStack at ??
0x108ee4d55: *raise<Exception>:NoReturn at ??
0x108ee4d01: *raise<String>:NoReturn at ??
0x108f126ff: *GC::enable:Nil at ??
0x108ee47f4: __crystal_main at ??
0x108ef48c8: main at ??

Seems pretty cryptic to me. The call for GC::enable is like 6 lines below.

Copy link
Member

@oprypin oprypin Sep 10, 2017

Choose a reason for hiding this comment

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

It's a whole another issue, but it doesn't have to be this way.

Copy link
Member Author

@asterite asterite Sep 10, 2017

Choose a reason for hiding this comment

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

What about "Can't enable GC: GC is not disabled". I mean, there's nothing wrong with being just a bit more verbose, right?

@lbguilherme
Copy link
Contributor

@lbguilherme lbguilherme commented Sep 10, 2017

I don't have an use case, but I think it is better to not raise any exception in this case and just return. GC.enable always finish with a enabled GC in the same way that file.close always finish with a closed file, even it it was already closed. This exception is not helpful as there was nothing really wrong happening.

@oprypin
Copy link
Member

@oprypin oprypin commented Sep 10, 2017

@lbguilherme
Copy link
Contributor

@lbguilherme lbguilherme commented Sep 10, 2017

Yes. I would implement it this way:

  def self.enable
    unless LibGC.is_disabled != 0
      return
    end
    LibGC.enable
  end  

@asterite
Copy link
Member Author

@asterite asterite commented Sep 10, 2017

But if you call GC.enable without calling GC.disbale, it's a bug. Bugs shouldn't be silently ignored.

@lbguilherme
Copy link
Contributor

@lbguilherme lbguilherme commented Sep 10, 2017

Doesn't closing a file twice fall under the same category of bug? I'm not against either behavior, just wanting to be consistent.

@asterite
Copy link
Member Author

@asterite asterite commented Sep 10, 2017

@lbguilherme

GC.enable
puts "Hello"
GC.disable

What do you expect that do to?

To me, after the last line, the GC should be enabled. But in fact, it won't be like that, with your proposal.

Closing a file twice is fine, it's an idempotent operation. Enabling a GC that's not disabled can lead to the code above, and then finding where the problem of a disabled GC is becomes super tricky.

RX14
RX14 approved these changes Sep 13, 2017
@sdogruyol
Copy link
Member

@sdogruyol sdogruyol commented Sep 14, 2017

LGTM?

@RX14 RX14 added this to the Next milestone Sep 15, 2017
@RX14 RX14 merged commit 6013227 into crystal-lang:master Sep 15, 2017
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants