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

Removing the requirement that Rails::Application be a singleton. #9655

Closed

Conversation

wangjohn
Copy link
Contributor

There doesn't seem to be a good reason that Rails::Application has to be a singleton, other than the fact that the app needs to get initialized somewhere and its convenient to make it a singleton.

Overview of Changes

In this PR, I've made it so that Rails is no longer a singleton, and multiple applications can be initialized in a single ruby process. The caveat is that these applications will share a single global configuration which is stored in Rails.config, which is created as soon as the first rails application is initialized.

This also means that Rails.config is available before the environment initialization occurs.

New Initialization Procedure

You can now initialize multiple rails applications. To do this, I've changed the initialization of an application to the following structure (you can see this inside of /config/application.rb).

class Application < Rails::Application
end

Application.new do
  config.time_zone = 'Eastern'
  config.blah = 'blah'
end

This means that you can call Application.new as many times as you would like. You don't have to actually create a subclass, and you can just call Rails::Application.new, but creating a subclass is backwards compatible with the way applications have been initialized in the past.

Configuring Applications

When you configure an application now, you will be changing a global configuration object stored in Rails.config. You can also configure this directly by doing something like this:

Rails.configure do |config|
  config.time_zone = 'Eastern'
end

Global Configuration

This PR still uses a global configuration for all rails app. Whenever a new application is created, it's configuration will be the same as the configuration of all the other rails applications. (This global config will be relaxed as I continue to work on this PR. The end goal is to make it so that each application has its own, separate configuration).

In addition, the Rails module stores the global rake tasks. These rake tasks are shared across all applications.

Backwards Compatibility

I've made sure that this PR preserves a lot of old functionality. Initializing and configuring an application in the old way is still possible. For example: inside /config/application.rb you can still define an application as follows:

class Application < Rails::Application
  config.time_zone = 'Eastern'
end

Application.initialize!

The call to Application.initialize! sets Rails.application to a new instance of Application, but cannot be called twice.

Other Changes

I've also gotten rid of the Railtie::Configurable module. This is because it currently forces all children of Rails::Railtie to include this module, no matter what. This force feeding doesn't seem like good practice, and prevents you from having any say in whether you want the module or not. Moving the guts of the Configurable module inside Railtie itself helps alleviate this issue.

attr_accessor :cache, :logger
attr_reader :application

def application=(app)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't expect Rails.application= to have that many side effects. Maybe we should have a Rails.define_application or something but I can see people setting Rails.application to a mock or something in tests and this is definitely going to break it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Other examples, imagine you are temporarily swapping Rails.application= in between tests. You don't want those callbacks to be run every time an application is set.

@josevalim
Copy link
Contributor

There are actually many reasons for application to be a singleton. If you have more than one application defined, which one do you serve via Rack? Which one Rails.application should point to?

Even more, most frameworks like Action Controller have global configurations, making it impossible to have two distinct applications running side by side.

So it makes complete sense for a Rails developer to perceive those as singleton. It may be worthy removing the limitations from the code though, in order for people to create more than one application for testing, but the amount of side effects that would leak from having more than one application in the same process is very likely to not make it worthy.

@josevalim
Copy link
Contributor

Also, regarding Railtie::Configuration, I think it was first added because the methods instance, config and so forth don't make sense to be called on Rails::Railtie. In fact, Rails::Railtie should not even have an instance. That's why the mixin, to enhance just child classes with this functionality.

On the other hand, that's how inheritance works. It makes sense to call AR::Base.establish_connection but it doesn't make any sense to call AR::Base.find(1) so I am ok with this change. It may be worthy considering that to mark Rails::Railtie as abstract to ensure it can't be initialized or something (similar to AR).

@wangjohn
Copy link
Contributor Author

@josevalim Ah I see. I've reverted my changes for making Rails::Application a singleton and have moved the add_lib_to_load_path! call back to inheritance. However, it still doesn't seem right that this method is called upon inheritance, since it doesn't seem like the natural place to add to the load path. I'll look at the code a bit more and see if I can find a good place.

I've also made it impossible to instantiate a Railtie by raising an error on initialization if the class is an abstract railtie (Rails::Railtie, Rails::Engine, or Rails::Application) and have added a test to make sure it works.

@steveklabnik
Copy link
Member

I'm all about not making things a singleton, but you're right that people might shoot themselves in the foot with it. I wonder if there's a good way to achieve it...

@tenderlove
Copy link
Member

@steveklabnik we continue to support Rails.application. People can treat it as a singleton, but there's no reason we should limit to once instance.

There are actually many reasons for application to be a singleton. If you have more than one application defined, which one do you serve via Rack? Which one Rails.application should point to?

The one that is instantiated in config.ru.

There are many reasons not to be a singleton, for example, creating multiple apps with different configuration values, mounting the same app in multiple places, being able to test the app under different configs without abandoning the process.

Forcing the app to be a singleton is harmful for testing an reusability. But we don't need to make singleton a requirement in order to give the app developer "one instance".

@josevalim
Copy link
Contributor

That's exactly my point, you cant create applications with different
configurations and mount them in the same process. All the configurations
are global, when you set action mailer to use smtp in the config
application, it is going to affect everything.

I am not saying this change is bad, I am just saying the motivation for the
change is not true. We currently don't have the structure to support
multiple applications in the same process.

Historically, we have been adding slowly features to engines that are able
to work coherently in the same process, there is still a good way to go
though.

José Valim
www.plataformatec.com.br
Founder and Lead Developer
*

@josevalim
Copy link
Contributor

Btw bro, you can't answer one of the three questions and get away with it.
;)

John, the reason lib is added in the inherited hook and the before
configuration hook is invoked as well is because, from the user
perspective, as soon as he inherits from Rails::Application in
config/application.rb, he is ready to configure it. I am aware though it
sucks in case you want to create an abstract class from Rails::Application.

Inheritance considered harmful. :(

José Valim
www.plataformatec.com.br
Founder and Lead Developer
*

@tenderlove
Copy link
Member

That's exactly my point, you cant create applications with different configurations and mount them in the same process.

Ya, we hung ourselves. You can't use the argument of "you can't create multiple apps" as an argument against allowing us to create multiple apps. You can create multiple apps, we just don't let you.

All the configurations are global, when you set action mailer to use smtp in the config application, it is going to affect everything.

Yes. We create a top level configuration object when the app boots that is shared among other aspects of the application. Calling new on the app should not configure the app, but the config should be passed to the constructor. Then we can share it among components.

I am not saying this change is bad, I am just saying the motivation for the change is not true. We currently don't have the structure to support multiple applications in the same process.

We don't currently have it, and @wangjohn is going to make it happen. That is the motivation.

Historically, we have been adding slowly features to engines that are able to work coherently in the same process, there is still a good way to go though.

How is an Engine different than an application? It just seems like more hacks around the fact that you can't instantiate an application multiple times. If apps weren't required to be singletons, they would essentially be the same as engines.

I'm incredibly surprised that I have to argue about the benefits of not using singletons. :-P

@josevalim
Copy link
Contributor

I talked to @tenderlove in private and we are agreeing on this.

None of us like that application is a singleton and we want to get rid of it. This is mostly a chicken-egg issue: 1) do we remove the restriction that our application is a singleton and then fix global state or 2) do we fix the global state and just then make the application not a singleton?

This is a good starting point. We should continue working on it as we go!

@blowmage
Copy link
Contributor

👍 Thanks for this!

@janko
Copy link
Contributor

janko commented Mar 17, 2013

Maybe you could also smash changes that deal with same things into one commit, so there is a prettier history :)

@wangjohn
Copy link
Contributor Author

@janko-m Yes, I'm in the process of rebasing everything and adding documentation and tests. I've added a bunch of changes to the original PR, so this isn't ready to merge yet. However, it should be ready very soon. I'll be back with an update on all the changes I've made.

@wangjohn
Copy link
Contributor Author

I've updated the description of this PR to reflect its current state. I've allowed multiple Rails::Application to be created by changing the way one instantiates a rails application. The new method allows you to create as many applications as you want, but all of these applications will have the same global config.

Tests have also been added to make sure that the PR creates the functionality that is actually expected.

reworking the way the main app is initialized.
instance method in railties. This stops you from creating a new
application whenever instance is called. Added global config to the
rails application.
application. Creating instance methods
linking up to class methods in order to handle this.
Also, changing the way we handle config. The Rails.config command will create
a global config if it doesn't already exist.
config/environments files. They now use the global +Rails.config+ and
are configured using the new +Rails.configure+ method.
initialized correctly. Also changing some documentation.
that each application that is cloned shares the same configuration,
routes, env_config, etc.
@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014
@rafaelfranca rafaelfranca removed this from the 5.0.0 [temp] milestone Apr 5, 2016
@rails-bot
Copy link

rails-bot bot commented Dec 18, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Dec 18, 2019
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

7 participants