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 defining object finalizer in Ruby 3.1 to reference a UUID string #74

Merged
merged 8 commits into from
Aug 28, 2023
Merged

Fix defining object finalizer in Ruby 3.1 to reference a UUID string #74

merged 8 commits into from
Aug 28, 2023

Conversation

vkononov
Copy link
Contributor

@vkononov vkononov commented Mar 15, 2022

Describe the change

Fixes the "undefined local variable or method `callback_queue' for FiniteMachine::Observer:Class (NameError)" exception thrown in Ruby 3.1.1

Why are we doing this?

In Ruby 3.1.1, accessing callback_queue from a finalizer causes a "undefined local variable or method `callback_queue' for FiniteMachine::Observer:Class (NameError)", and callback_queue is never cleaned up. However, making the finalizer an instance method rather than a class method throws a warning about improper use of finalizers.

The fix is borrowed from a Github issue found at https://github.com/appsignal/rdkafka-ruby/pull/160/files.

Benefits

This library will become compatible with Ruby 3.1.1.

Drawbacks

None.

Requirements

  • Tests passing locally?
  • Code style checked?
  • Rebased with master branch?
  • Documentation updated?
  • Changelog updated?

In Ruby 3.1.1, accessing callback_queue from a finalizer causes a "undefined local variable or method `callback_queue' for FiniteMachine::Observer:Class (NameError)", and callback_queue is never cleaned up. However, making the finalizer an instance method rather than a class method throws a warning about improper use of finalizers.

The fix is borrowed from a Github issue found at https://github.com/appsignal/rdkafka-ruby/pull/160/files.
@ghn ghn mentioned this pull request Apr 28, 2022
@dup2
Copy link

dup2 commented Jul 1, 2022

This just hit me also when trying to update an application to ruby 3.1.

What do we need to be able to merge this?

@luiswitz
Copy link

luiswitz commented May 5, 2023

Hello 👋 - Re-asking @dup2 question: what do we need to be able to merge this?

Copy link
Owner

@piotrmurach piotrmurach left a comment

Choose a reason for hiding this comment

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

Hi Vadim,

Thank you for the PR and for resolving this issue. Would you have time to look into the comments? I appreciate this is long overdue so hopefully, we can get this sorted for everyone.

Also, if you could add an entry in the changelog under the 'Fixed' heading with your name and handle that would be great. Please don't worry about the version, you can use '## unreleased' for now.

lib/finite_machine/observer.rb Show resolved Hide resolved
lib/finite_machine/observer.rb Outdated Show resolved Hide resolved
@vkononov
Copy link
Contributor Author

@piotrmurach Please let me know if there's anything else you need me to do to get this merged. Thanks.

Copy link
Owner

@piotrmurach piotrmurach left a comment

Choose a reason for hiding this comment

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

Hi, I've left a few more comments. The annoying thing is that this finalizer is only necessary when async callbacks are used. Otherwise, for the vast majority of cases, I believe it shouldn't be even defined. Eager to learn about what you think.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
lib/finite_machine/observer.rb Outdated Show resolved Hide resolved
lib/finite_machine/observer.rb Outdated Show resolved Hide resolved
lib/finite_machine/observer.rb Outdated Show resolved Hide resolved
lib/finite_machine/observer.rb Outdated Show resolved Hide resolved
@vkononov
Copy link
Contributor Author

vkononov commented Jul 6, 2023

I've made a bit of refactoring to address your comments and after familiarizing myself further with how the ObjectSpace class works. Hopefully, this can be merged now, but I'm happy to hear any other feedback.

@abhishekjain16
Copy link

abhishekjain16 commented Aug 3, 2023

@piotrmurach Is there anything we can do to get this merged and make this lib compatible with Ruby 3.1+. Happy to help in any way possible

@vkononov
Copy link
Contributor Author

vkononov commented Aug 4, 2023

@piotrmurach Is there anything we can do to get this merged and make this lib compatible with Ruby 3.1+. Happy to help in any way possible

@abhishekjain16 You could help resolve the failing pipelines. That's probably the only thing that is still needed.


@callback_queue = MessageQueue.new
@object_id = SecureRandom.uuid
ObjectSpace.define_finalizer(@object_id, cleanup_callback_queue)
Copy link

@abhishekjain16 abhishekjain16 Aug 7, 2023

Choose a reason for hiding this comment

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

@vkononov This method needs to return @callback_queue

      return @callback_queue if instance_variable_defined?(:@callback_queue)

      @callback_queue = MessageQueue.new
      @object_id = SecureRandom.uuid
      ObjectSpace.define_finalizer(@object_id, cleanup_callback_queue)
      @callback_queue

Add @callback_queue to return from this method fixes pipeline

@abhishekjain16
Copy link

@vkononov https://github.com/piotrmurach/finite_machine/pull/74/files#r1285879315 This should fix the pipeline

@vkononov
Copy link
Contributor Author

vkononov commented Aug 8, 2023

Thank you, @abhishekjain16. Can't believe I missed that. @piotrmurach, Could you run the pipeline and merge this code if the pipeline succeeds?

@piotrmurach piotrmurach changed the title Change how the callback queue is cleared from the finalizer. Fix defining object finalizer in Ruby 3.1 to reference a UUID string Aug 20, 2023
@piotrmurach piotrmurach linked an issue Aug 28, 2023 that may be closed by this pull request
@piotrmurach piotrmurach merged commit 11dd7f1 into piotrmurach:master Aug 28, 2023
11 of 12 checks passed
@jimbali
Copy link

jimbali commented Aug 30, 2023

Thanks for this. Are there any imminent plans to release a new version to RubyGems with this fix?

@piotrmurach
Copy link
Owner

@jimbali Thanks for your interest. I'm preparing a new release but cannot promise any timelines.

@jimbali
Copy link

jimbali commented Sep 28, 2023

Hi @piotrmurach, have you found any time to prepare a release for this yet? Any idea when that might be? Thanks

@piotrmurach
Copy link
Owner

Hi @jimbali, I appreciate your patience. I had to deal with upsetting news from my family that didn't allow me the headspace to finish what I wanted. I released a new 0.14.1 version with a few fixes. Please see the changelog for more information.

@jimbali
Copy link

jimbali commented Oct 10, 2023

Aw man, really sorry to hear that. Thanks for finding the time to sort this out. I've sponsored you a couple of coffees!

@piotrmurach
Copy link
Owner

@jimbali That's life and the show must go on. You didn't have to but thank you. Appreciate your kindness. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Errors with Ruby 3.1.2
6 participants