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

Autoloading and Rails #235

Closed
pmorton opened this issue Apr 8, 2016 · 5 comments
Closed

Autoloading and Rails #235

pmorton opened this issue Apr 8, 2016 · 5 comments
Assignees
Labels

Comments

@pmorton
Copy link

pmorton commented Apr 8, 2016

We are getting some strange exceptions from the recurly railtie each time we deploy our application. The app is rails 4.2 with puma running with 2 clustered mode workers with 16 threads. When the application is deployed the first call into our app (any controller) will trigger the following exception: NoMethodError: undefined method accept_language=' for Recurly::API:Class` and a massive stack trace in what looks like the request filter phase. This generally does not make sense because the method is defined. After much research, it seems that this is probably an issue with the use of autoload.

https://bugs.ruby-lang.org/issues/921 confirms that for 1.9.3 and older, autoload is not thread safe while 2.x is. Further @headius seems to indicate that autoload is only thread safe is the same autoload mechanism is used across the board. headius/thread_safe#15 (comment). So here is where I believe that issue to be: Rails uses it's own autoloading systems, while this gem uses another. There is some race condition that is causing the Recurly::API class to only be partially loaded when it is called.

To address this issue we could take one of a couple of approaches

  1. Dump autoloading and use require. This would make the gem slower to load, but this should be trivial since the gem is pretty small.
  2. Hook into the rails autoloading system. Each file using autoloads would add the following to the class
    extend ActiveSupport::Autoload if defined?(ActiveSupport::Autoload). The net result is that the activesupport autoloader which is used by rails would also be used by this gem. Doing this would allow lazy loading for ruby files in development and eager loading in production.

I believe that 1 is the simplest with very little performance tradeoff, but if load performance is important to you option 2 would work as well. Let me know what you think. I am happy to code this up.

@bhelx bhelx self-assigned this Apr 8, 2016
@pmorton
Copy link
Author

pmorton commented Apr 11, 2016

Good Monday @bhelx Let me know your thoughts on the above and I would be happy to get this moving.

@bhelx
Copy link
Contributor

bhelx commented Apr 11, 2016

Thanks @pmorton

Will be taking a look today

@bhelx
Copy link
Contributor

bhelx commented Apr 11, 2016

@pmorton

Okay, from what I can tell, our Railtie is pretty old and this library was written when using threaded servers was less common. So it's no wonder there are some issues.

At first glance I would definitely say I want to dump autoload (option 1). I find messing with ruby's requires to be a little to opaque and I never liked that this library adopted autoload. I also agree that the performance penalty will be negligible. I'd be happy to look over your PR or help you get it working.

@pmorton
Copy link
Author

pmorton commented Apr 11, 2016

Great. I will get this moving. Cheers!

@bhelx
Copy link
Contributor

bhelx commented Apr 29, 2016

Closed with #236

Thanks @pmorton

@bhelx bhelx closed this as completed Apr 29, 2016
@bhelx bhelx added the V2 V2 Client label Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants