Skip to content
This repository has been archived by the owner on Apr 1, 2023. It is now read-only.

Problem loading job classes with autoloading #151

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

noiseunion
Copy link

If you pass a string value in for the class name, the Module.const_defined? will not return true if the constant has not yet been loaded. In a Rails environment this can be a bummer due to the lazy loading in Rails. To address this, we can instead attempt to load the const and if a NameError occurs, we'll just return nil as the original code would have done.

I ran into this when using the #push_bulk feature of Sidekiq. Jobs created using that method did not get created properly as status workers if the class name was a string and not yet loaded.

JD Hendrickson added 2 commits May 31, 2019 11:04
If you pass a string for the job class, and the constant has already been loaded, the world is a happy place.  However, if the constant has not yet been loaded (lazy loading in Rails for example) then this breaks.  This fix attempts to autoload the constant to fix this issue.
resolve job classes that have not been loaded yet
@kenaniah
Copy link
Collaborator

kenaniah commented Jun 1, 2019

Hi and thanks for the PR! Would you be able to write one or more tests proving the new code works as expected? Hopefully those tests can fail against master but pass on your branch.

Earliest I'd be able to write those myself would probably be early next week, but if you could provide them, that would be super helpful.

@noiseunion
Copy link
Author

Hey @kenaniah - sorry for the delay. I'll see what I can do to get some additional tests in place this week.

@noiseunion
Copy link
Author

@kenaniah - just an update that I have not forgotten about this - I just have not been able to get to it yet. Hoping I can in the next few days.

@jkeen
Copy link

jkeen commented Feb 28, 2020

@noiseunion I think you forgot about this 😛

@noiseunion
Copy link
Author

@jkeen - yes I did forget!!! 😬 Sorry about that. I will make some time this week to write some tests.

@noiseunion
Copy link
Author

UPDATE - I have started trying to write tests for this. However, in all honesty, I have not yet figured out how best to do so. Somehow I have to for lazy loading of class within the test and I honestly am not really sure how best to do that at the moment.

I am definitely open to any suggestions 😄

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

Successfully merging this pull request may close these issues.

3 participants