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.

Sign up for GitHub

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

Jump to bottom

Load ShoryukenConcurrentSendAdapter when loading Rails #642

Merged
merged 1 commit into from
Dec 28, 2020
Merged

Load ShoryukenConcurrentSendAdapter when loading Rails #642

merged 1 commit into from
Dec 28, 2020

Conversation

cjlarose
Copy link
Collaborator

Fixes #641. Technical details in the commit message.

@cjlarose
Load ShoryukenConcurrentSendAdapter when loading Rails
6851869 
ShoryukenConcurrentSendAdapter is currently only loaded from
`lib/shoryuken.rb` if `Shoryuken.active_job?` is truthy. When
`lib/shoryuken.rb` is loaded, though, it's possible that the constant
`ActiveJob` has not yet been defined. If that happens, users can
encounter errors related to the constant ShoryukenConcurrentSendAdapter
not being initialized when ActiveJob tries to configure itself.

It is therefore sometimes necessary to wait until we load the rails
application (specifically `config/application.rb`) before we check
`Shoryuken.active_job?`. If that's truthy, then we can load
`active_job_concurrent_send_adapter`.
@pr-link pr-link bot mentioned this pull request Dec 28, 2020
Closed
@phstc phstc self-requested a review December 28, 2020 05:48
phstc
phstc approved these changes Dec 28, 2020
View reviewed changes
Copy link
Collaborator

@phstc phstc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@cjlarose
Copy link
Collaborator Author

@phstc I think we might be able to revisit these lines in lib/shoryuken.rb in a future change, since the functionality that it adds essentially depends on load order (the ActiveJob constant must have already been defined before shoryuken.rb runs). I think those lines might be important if you're using ActiveJob, but not using Rails (so you don't have -R passed to shoryuken), but I'm not 100% certain.

https://github.com/phstc/shoryuken/blob/4a03b64a4c48275d83c69b82c60c1379d9a350dc/lib/shoryuken.rb#L91-L94

Either way, I think the proposed change is relatively low risk since it just adds a new require for some situations where it's necessary.

@cjlarose cjlarose merged commit c60598a into ruby-shoryuken:master Dec 28, 2020
@phstc
Copy link
Collaborator

phstc commented Dec 30, 2020

@cjlarose thanks for quickly addressing this 🚀

I think those lines might be important if you're using ActiveJob, but not using Rails (so you don't have -R passed to shoryuken), but I'm not 100% certain.

I think that if is for the other way around if you are using Rails but without ActiveJob. But I'm not sure if that was/is posible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phstc phstc phstc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NameError: uninitialized constant ActiveJob::QueueAdapters::ShoryukenConcurrentSendAdapter
2 participants
@cjlarose @phstc