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

RFC: Simplify code reloading by way of fs-events and forking #24981

Closed
tenderlove opened this issue May 11, 2016 · 16 comments
Closed

RFC: Simplify code reloading by way of fs-events and forking #24981

tenderlove opened this issue May 11, 2016 · 16 comments

Comments

@tenderlove
Copy link
Member

tenderlove commented May 11, 2016

Our auto-reload code has always been a pain for maintenance and it's very difficult to understand. I'd like to propose a change to the way we do reloading.

General idea

Run a forking server in development. The parent process can pre-load framework code and gems. Child processes will load application code via normal methods like require and autoload. If our FS monitor detects anything changes and a reload needs to occur, it can send a signal to the parent process and have it reap it's children. The newly born children will load the new code since the parent process didn't have any of the app code loaded.

I think we only need to do this for models / controllers (or anything that is in the list in the config). Views don't need to cause a reload because I think we already handle that in a different way.

Challenges

  1. We've talked about this many years ago, and there were blockers, but I don't remember what they were. Maybe we don't have those blockers today.
  2. How will this work with ActiveJob and ActionCable? Presumably we can tell ActiveJob to empty it's queue before we reap children, or we can ensure that any process that is doing jobs doesn't service requests (IOW, children currently being reaped won't service requests, but we can fork new children that do at the same time). ActionCable, I have no idea.
  3. Platforms that don't support fork (like JRuby). We should probably make our "code reload" a strategy object, and we can keep the old strategy for other platforms, until we figure out a better solution (or let platform maintainers deal with it).
  4. We need to disconnect from the database when reaping children.
  5. Is forking fast enough? Can we create new child processes that are ready to serve requests after someone writes to a file but before they try to load the page (are they going to notice that we're killing / starting processes while doing normal dev work)?

All of the challenges I've thought of don't really seem like blockers to me. Are there any real blockers for doing this? I think it would greatly simplify our reloading system.

@tenderlove tenderlove added this to the 5.1.0 milestone May 11, 2016
@jeremy
Copy link
Member

jeremy commented May 11, 2016

REP #1

@maclover7
Copy link
Contributor

I will look into "strategizing" the current file update checker, and trying to provide a nicer public API for people to implement.

@fxn
Copy link
Member

fxn commented May 11, 2016

The main issue I see is that one of the main goals of autoloading is precisely not to have require calls. Just program as if the app was preloaded. This is one of the original motivations of @dhh.

Autoloading would be way easier and would match Ruby semantics if Kernel#autoload worked for our use case. The idea would be to walk config.autoload_paths and issue autoload calls in Object or namespaces, but there are a number of gotchas. Some of them can be hacked around, but there's at least one that involves namespaces that does not seem to be possible to avoid.

@tenderlove
Copy link
Member Author

@fxn I'm not proposing that we make any change about using autoload or require. We keep all those the same. I mean we quit keeping track of what constants to reload when a file changes and just let forking take care of it.

@fxn
Copy link
Member

fxn commented May 11, 2016

@tenderlove Ah! I interpreted that

Child processes will load application code via normal methods like require and autoload

meant that child code would use manual requires to load application code and do not autoload, only code reload would be implemented by forking again.

@tenderlove
Copy link
Member Author

@fxn right. I don't want to mess with any requires or autloads, only touch our reloading stuff. 😄

@thedarkone
Copy link
Contributor

Platforms that don't support forking generally don't do so because they have parallelism and things like JITs. Isn't Rails (or Ruby) moving in a direction of having or relying on those things?

Views don't need to cause a reload because I think we already handle that in a different way.

Yes, views are handled differently, but forking will always bust their caches (evaled templates).

@tenderlove
Copy link
Member Author

Platforms that don't support forking generally don't do so because they have parallelism and things like JITs. Isn't Rails (or Ruby) moving in a direction of having or relying on those things?

Possibly MRI will have a JIT in the future. But I don't think that worrying about that is a good reason to block this. Our reloading code is a notorious for headaches and if leveraging tools like fork will help us simplify it, then I think we should use those tools. I don't want to ignore a technology based on "what-ifs".

Regardless, if we use a strategy pattern, then we can switch in the event that MRI stops supporting fork, and JRuby can continue to use the non-forking strategy.

@fxn
Copy link
Member

fxn commented May 11, 2016

Another possible challenge I see (at least as things work today) is that routes are loaded during the initialization process, and they may autoload constants. Not saying it cannot be addressed, only adding to the list of gotchas.

@sgrif
Copy link
Contributor

sgrif commented May 16, 2016

We should probably make our "code reload" a strategy object, and we can keep the old strategy for other platforms, until we figure out a better solution

If the main goal is to simplify the code base, doesn't having to keep two implementations defeat that entirely? It should be noted that windows doesn't support fork either, and we're the platform maintainers there.

@tenderlove
Copy link
Member Author

@sgrif we're also having problems with deadlocking. On platforms that don't support fork, we can also use spawn. Spawners won't be as fast to boot, but should work on windows.

@sgrif
Copy link
Contributor

sgrif commented May 16, 2016

Seems reasonable for the short term. Would it be possible to for us to maybe look at getting something upstream which could help us accomplish this with threads? It seems like we need some way to approximate run-to-completion semantics, either by being able to block on constant access, or by being able to lock on require...

@tenderlove
Copy link
Member Author

@sgrif @matthewd is working with upstream, but so far the answer is "no".

@sgrif
Copy link
Contributor

sgrif commented May 17, 2016

😞

@rafaelfranca rafaelfranca modified the milestone: 5.1.0 Feb 23, 2017
@rails-bot rails-bot bot added the stale label May 24, 2017
@rails-bot
Copy link

rails-bot bot commented May 24, 2017

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@maclover7 maclover7 added pinned and removed stale labels May 24, 2017
@byroot
Copy link
Member

byroot commented May 6, 2021

I believe this is no longer necessary thanks to Zeitwerk.

@byroot byroot closed this as completed May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants