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

Issues with sidekiq not seeing all the associations. #15

Open
urkle opened this issue Feb 14, 2016 · 11 comments
Open

Issues with sidekiq not seeing all the associations. #15

urkle opened this issue Feb 14, 2016 · 11 comments

Comments

@urkle
Copy link
Member

urkle commented Feb 14, 2016

So I have a rails application and am making use of this gem to automatically build up my associations (which is OH so awesome BTW).. However I added a new sidekiq job which is fails the first time it is run due to the associations not being created yet.

Scenario

  • a rails production environment
  • a Client model
  • an AccessToken model which has a foreign key to client (via a client_id column)
  • a sidekiq job that takes a client_id and user_id (user id is irrelevant for the purposes of this bug)
    • this job loads up the client Client.find(client_id) and attempts to use the access_tokens association

What happens

  • an error is raised as there is no access_tokens association (yet).

So.. any ideas on how I can get this gem to generate all of the associations on sidekiq startup so I don't run into this issue?

ronen added a commit that referenced this issue Mar 6, 2016
@ronen
Copy link
Member

ronen commented Mar 6, 2016

@urkle sorry for the delay in responding.

I had thought that the problem was that if a model is loaded up using find before anything else, that doesn't trigger the trigger the automatic association generation, and I even wrote a failing spec. Then when I went in to fix it I realized that my spec was wrong, and indeed find does work. And so, I don't know what's going on in your case.

Have you managed to figure it out?

One possible workaround to try, if you have an opportunity to run an initializer of some sort before sidekiq starts the jobs: run Client.reflect_on_all_associations which triggers schema_assocations to do its stuff. That shouldn't be needed, but if it helps maybe that'd provide a clue?

@urkle
Copy link
Member Author

urkle commented Mar 16, 2016

reflect_on_all_associations didn't work (it blew up with exceptions about not being able to find method schema_validations in my class)..

However I think I have a clue as to why it is occurring. As this only happens some of the time. and generally after one job fails it then works from then on.

Sidekiq is threaded so the associations NEED to be built up before sidekiq spawns threads. As it is right now it's lazy and they get created on first use. I have a method in the code that spawns multiple instances of the same job at the same time. When all of these are running they all are trying to create the associations and there's a race.. where one of them always fails.

@urkle
Copy link
Member Author

urkle commented Mar 16, 2016

OK.. so if I add this to an initializer. Then it works.. However this ideally needs to be something in the core of schema_validations and schema_associations so that it works on startup., Ideally this needs to be done not only for Sidekiq usage but also for production Rails usage! (for users who use threaded servers like puma or passenger)

Sidekiq.configure_server do |c|
  ActiveSupport.on_load(:active_record) do
    SchemaAssociations.insert
    SchemaValidations.insert
    [ClientApplication, User, ...].each do |m|
      m.reflect_on_all_associations
      m.validators
    end
  end
end

@ronen
Copy link
Member

ronen commented Mar 16, 2016

Oh duh! SchemaAssociations wasn't being initialized at all. That explains it :)

As it turns out I've got a fix for this already merged but not released for SchemaAssociations (see #17), and will do the same fix for SchemaValidations. With any luck I'll have them both released tomorrow. In the meantime if you want you can try running SchemaAssociations from master and drop SchemaAssociations.insert and m.reflect_on_all_associations from the initializer.

@ronen
Copy link
Member

ronen commented Mar 17, 2016

OK, I've released schema_associations 1.2.5 and schema_validations 2.0.2, upgrading both to use SchemaMonkey, which inserts things properly. Hopefully you can now drop that initializer entirely and everything should be hunky-dory.

Let me know...

@urkle
Copy link
Member Author

urkle commented Mar 25, 2016

@ronen I upgraded to the latest schema associations & Validations and dropped my initializer hack and things appear to be working fine now. I'm closing this now. Thanks for getting this fixed!

@urkle urkle closed this as completed Mar 25, 2016
@ronen
Copy link
Member

ronen commented Mar 28, 2016

@urkle great, glad it's working now!

@urkle
Copy link
Member Author

urkle commented Mar 28, 2016

So.. it was working.. and we deployed everything to production saturday evening. However today, I received another error about an undefined association.. It also seems that the associations are still lazily loaded. (e.g. if Rails.configuration.cache_classes is true it should run the setup on startup and not be lazy)

@urkle urkle reopened this Mar 28, 2016
@ronen
Copy link
Member

ronen commented Mar 28, 2016

@urkle bummer. can you give a stack trace or anything with more info?

But yes, even if classes are cached, the associations are still lazily defined, with the intention of defining them the first time they're needed. (This is what ActiveRecord itself does for defining attribute accessors and whatnot.) It's possible though that schema_associations has somehow missed detecting "the first time they're needed" for some case(s).

@urkle
Copy link
Member Author

urkle commented Mar 29, 2016

stack trace isn't real helpful at all. It's just failing in attribute_methods.rb:433
Before that is just my code accessing the association.

In this case I received 3 errors at the same exact time.. So it's still the threaded issue and the fact that they are lazily loaded.. Lazy loading does not work with threads. So I have to add back in my preloader.

def schema_plus_preload
  ActiveSupport.on_load(:active_record) do
    [ClientApplication,User,..otherClasses...].each do |m|
      m.reflect_on_all_associations
      m.validators
    end
  end
end

if Rails.configuration.eager_load
  schema_plus_preload
else
  Sidekiq.configure_server do |c|
    schema_plus_preload
  end
end

This will ensure everything is loaded before my sidekiq starts spawning processes.. And also be loaded when eager loading is enabled so we can better optimize in a forking (COW) and/or threaded webserver.

@ronen
Copy link
Member

ronen commented Mar 31, 2016

I've spotted something that could be a threading issue: the code was setting the "have defined the associations flag" at the top of the method that defines the associations rather than at the bottom. No difference when running in a single thread, but when multithreaded that could of course lead to an incorrect state, where thread A sets the flag and thread B sees the flag before A finishes so assumes the associations are there, then fails when they're not. (By waiting til the end both A and B would define the same associations in parallel, but that's harmless.)

Anyway that's just a shot in the dark; I don't currently have a test harness to reproduce the bug. But it seems like an obvious thing to fix.

I've pushed it to master. Would be able to test it out without causing yourself undue grievance from production failures?

Thanks

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

No branches or pull requests

2 participants