SchemaPlus / schema_associations

ActiveRecord extension that automatically (DRY) creates associations based on the schema
Other
46 stars 8 forks source link

Issues with sidekiq not seeing all the associations. #15

Open urkle opened 8 years ago

urkle commented 8 years ago

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

What happens

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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!

ronen commented 8 years ago

@urkle great, glad it's working now!

urkle commented 8 years ago

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)

ronen commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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