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

Create RactorLocalSingleton #4

Merged
merged 5 commits into from
Oct 1, 2024
Merged

Create RactorLocalSingleton #4

merged 5 commits into from
Oct 1, 2024

Conversation

rm155
Copy link
Contributor

@rm155 rm155 commented Aug 18, 2021

Classes that include Singleton cannot be used within Ractors, since the instance can only exist within the main Ractor. By creating RactorLocalSingleton, classes have the option of instead having one instance per Ractor.

@ioquatix
Copy link
Member

ioquatix commented Aug 19, 2021

I'm not sure we should be encouraging the Singleton design pattern. We should probably encourage thread-local singletons which are more robust and safe.

lib/singleton.rb Outdated
@@ -93,21 +93,25 @@
#
module Singleton
VERSION = "0.1.1"
VERSION.freeze
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional? I think VERSION = "0.1.1".freeze is the correct change.

Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we just using frozen_string_literal: true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you both! I meant to clean that up. I've changed it, and it should be fixed now!

Copy link
Member

Choose a reason for hiding this comment

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

👍

@ioquatix
Copy link
Member

Does this imply that singletons are no longer singleton? Does this mean all users of Singleton need to rewrite their code to work with Ractor? I don't understand the use case. Are there some examples where we would adopt RactorLocalSingleton?

@rm155
Copy link
Contributor Author

rm155 commented Sep 19, 2021

Does this imply that singletons are no longer singleton? Does this mean all users of Singleton need to rewrite their code to work with Ractor? I don't understand the use case. Are there some examples where we would adopt RactorLocalSingleton?

I agree that RactorLocalSingletons would not be true Singletons—they are Singleton only in the context of their own Ractor.

If users want to be able to use an existing Singleton-based class within a Ractor, they would just change “include Singleton” to “include RactorLocalSingleton”. I don’t think any other change would necessary in most cases.

I think that the main use case would be for classes that include Singleton only for the sake of syntax and organization (rather than the condition of being completely singular). For example, the Prime library currently defines the Prime class using Singleton. I believe that the reason is solely to provide a specific structure for calling Prime-related methods. In order to make Prime completely Ractor-compatible, it cannot be a true Singleton. Rewriting Prime so that it doesn’t use Singleton at all would probably change too many properties of the library. By just using RactorLocalSingleton instead, it can become Ractor-compatible while retaining its essential properties.

Again, I agree that this is not a true Singleton. But I think that having this option may encourage more libraries to become Ractor-compatible.

@ioquatix
Copy link
Member

Why wouldn't you just change Singleton to be Ractor compatible? Are you essentially saying that every place we write include Singleton now needs to be updated to include RactorLocalSingleton otherwise it's basically broken?

@rm155
Copy link
Contributor Author

rm155 commented Sep 19, 2021

Why wouldn't you just change Singleton to be Ractor compatible? Are you essentially saying that every place we write include Singleton now needs to be updated to include RactorLocalSingleton otherwise it's basically broken?

I don’t think Singleton itself can be made Ractor-compatible—if it were, then there would be multiple instances of it (one per Ractor), so it wouldn’t be a true Singleton anymore. This could potentially be problematic for some classes relying on this attribute.

Moving a class from Singleton to RactorLocalSingleton should allow the class to become usable in Ractors. But if the class doesn’t need to be used in Ractors, I don’t think it needs to be changed.

@ioquatix
Copy link
Member

Moving a class from Singleton to RactorLocalSingleton should allow the class to become usable in Ractors. But if the class doesn’t need to be used in Ractors, I don’t think it needs to be changed.

How can "class doesn't need to be used in Ractors" be known ahead of time? Surely with the introduction of Ractor, ALL usage of Singleton is now broken and need to be updated/changed?

@ko1
Copy link
Contributor

ko1 commented Nov 8, 2021

This proposal is to introduce "Singleton object per Ractor".
Not replacement of the all usage of Singleton, but some of usage can be replaced with it, like computation cache.

@ko1
Copy link
Contributor

ko1 commented Nov 8, 2021

On 2.4 it seems failed. @rm155 could you check it?

@rm155
Copy link
Contributor Author

rm155 commented Nov 8, 2021

On 2.4 it seems failed. @rm155 could you check it?

@ko1 Okay, I think I have addressed the issue. I hope it's better now!

@ioquatix
Copy link
Member

ioquatix commented Nov 8, 2021

I believe this PR needs more discussion. Is there a bugs.ruby-lang.org issue?

This proposal is to introduce "Singleton object per Ractor".

@ko1 Where is the proposal?

@ko1
Copy link
Contributor

ko1 commented Nov 8, 2021

https://bugs.ruby-lang.org/issues/18127
and it is already accepted.

@hsbt hsbt merged commit 6780da5 into ruby:master Oct 1, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants