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

Fix auto extend error when using an ActiveJob wrapper #213

Closed
wants to merge 3 commits into from
Closed

Fix auto extend error when using an ActiveJob wrapper #213

wants to merge 3 commits into from

Conversation

takeyuweb
Copy link

When using auto extend

ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper
Could not auto extend the message ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper/<queue_name>/ visibility timeout. Error: undefined method `[]' for nil:NilClass

Fix auto extend error when using an ActiveJob wrapper
6c46aae 
ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper
Could not auto extend the message ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper/<queue_name>/<message> visibility timeout. Error: undefined method `[]' for nil:NilClass
houndci-bot
houndci-bot reviewed May 23, 2016
View reviewed changes
lib/shoryuken/util.rb
@@ -42,7 +42,8 @@ def worker_name(worker_class, sqs_msg, body = nil)
&& !sqs_msg.is_a?(Array) \
&& sqs_msg.message_attributes \
&& sqs_msg.message_attributes['shoryuken_class'] \
&& sqs_msg.message_attributes['shoryuken_class'][:string_value] == ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper.to_s
&& sqs_msg.message_attributes['shoryuken_class'][:string_value] == ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper.to_s \
Copy link

Choose a reason for hiding this comment

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

Line is too long. [138/120]

houndci-bot
houndci-bot reviewed May 23, 2016
View reviewed changes
lib/shoryuken/util.rb
@@ -42,7 +42,9 @@ def worker_name(worker_class, sqs_msg, body = nil)
&& !sqs_msg.is_a?(Array) \
&& sqs_msg.message_attributes \
&& sqs_msg.message_attributes['shoryuken_class'] \
&& sqs_msg.message_attributes['shoryuken_class'][:string_value] == ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper.to_s
&& sqs_msg.message_attributes['shoryuken_class'][:string_value] == \
ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper.to_s \
Copy link

Choose a reason for hiding this comment

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

Align the operands of a condition in an if statement spanning multiple lines.

phstc
phstc reviewed May 30, 2016
View reviewed changes
lib/shoryuken/util.rb
&& sqs_msg.message_attributes['shoryuken_class'][:string_value] == ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper.to_s
&& sqs_msg.message_attributes['shoryuken_class'][:string_value] \
== ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper.to_s \
&& body
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @takeyuweb

So can body be empty when `ActiveJob?

@takeyuweb
Copy link
Author

Hi @phstc
It is here.
https://github.com/phstc/shoryuken/blob/6a3c94df1a56f4cca8bf900e48c43935b1ac1df7/lib/shoryuken/middleware/server/auto_extend_visibility.rb#L30

@phstc
Copy link
Collaborator

phstc commented May 30, 2016

It is here.
https://github.com/phstc/shoryuken/blob/6a3c94df1a56f4cca8bf900e48c43935b1ac1df7/lib/shoryuken/middleware/server/auto_extend_visibility.rb#L30

@takeyuweb hm good catch. I'm wondering if we should pass the body in there, otherwise the worker_name will return JobWrapper instead of the job class.

cc/ @mariokostelac

@mariokostelac
Copy link
Contributor

mariokostelac commented Jun 1, 2016 •
edited
Loading

New code won't break, that's for sure. As far as I understand, ActiveJob can't be represented by a blank body.
If we look at ActiveJob::Base serialisation (https://github.com/rails/rails/blob/52ce6ece8c8f74064bb64e0a0b1ddd83092718e1/activejob/lib/active_job/core.rb#L75-L84), we can see that it has to be a valid json, one with job_class set.
Was blank message representing a valid ActiveJob at some point?

EDIT:
Ok I got it. I did not see that part of passing body. Body can't be blank.

@mariokostelac
Copy link
Contributor

I think we can just remove calling worker_name for the purpose of logging. Since worker_name does not work for batch workers either, it does not bring extra value.
We can just log queue name and message handle (or some sort of identifier).

phstc pushed a commit that referenced this pull request Sep 2, 2016
Complementary fix for #213. Pass body to worker_name for calls from A…
86497ec 
…utoExtendVisibility
@phstc phstc mentioned this pull request Sep 2, 2016
Merged
phstc pushed a commit that referenced this pull request Sep 2, 2016
Complementary fix for #213. Pass body to worker_name for calls from A…
d745f74 
…utoExtendVisibility
@phstc
Copy link
Collaborator

phstc commented Sep 2, 2016

@takeyuweb thanks, I could check it deeper now and I understood. I branched from your branch and created #240 with your fix, and also starting to pass along body from the auto extender middleware to the method worker_name. Actually, checking Shoryuken code base, only the auto extender and timing middlewares use worker_name - TBH I would love to remove that method, I understand it's handy to see more details in the log, but not everybody use those middlewares. I think it could simplify a bit removing it, but as it's public on Util, I can't remove it without introduce a major release.

Closing in favor of #240.

@phstc phstc closed this Sep 2, 2016
phstc pushed a commit that referenced this pull request Sep 2, 2016
Complementary fix for #213. Pass body to worker_name for calls from A…
a5ec345 
…utoExtendVisibility
phstc pushed a commit that referenced this pull request Sep 2, 2016
Complementary fix for #213. Pass body to worker_name for calls from A…
922449c 
…utoExtendVisibility
phstc added a commit that referenced this pull request Sep 2, 2016
@phstc
Complementary change for #213 (#240)
3bd598d 
* Fix auto extend error when using an ActiveJob wrapper

ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper
Could not auto extend the message ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper/<queue_name>/<message> visibility timeout. Error: undefined method `[]' for nil:NilClass

* Fix `Line is too long. [138/120]`

* Fix for `Align the operands of a condition in an "if statement" spanning multiple lines.`

* Fix initialization when the Load Rails option is specified

* Add rspec task

See https://www.relishapp.com/rspec/rspec-core/docs/command-line/rake-task.

* Fix NameError exception in AutoExtendVisibility

When execute command `bundle exec shoryuken -C config/shoryuken.yml --rails`.

> uninitialized constant Shoryuken::Middleware::Server::AutoExtendVisibility::MessageVisibilityExtender::Celluloid (NameError)

* Reduce shutdown wait time

* Run dispatch when processor done or dies

* Daemonize before loading environment

Daemonizing forks the process, which kills all other running threads. If
your Rails app starts threads during initialization, they will currently
be killed almost immediately.

* Document producer setup (#188)

* Add release date back on the change log

* Bump version to 2.0.7

* Add new author

* Load Celluloid before environment

* Bump version to 2.0.8

* Fix error when initialize shoryuken daemonize

* Fix daemonization broken in #219

* Bump version to 2.0.9

* Revert "Run dispatch when processor done or dies" (#226)

This reverts commit f4640d9.

* Bump version to 2.0.10

* I'm a completely stupid and removed the wrong version in rubygems

* Update CHANGELOG to add 2.0.11

* Minor sample default_worker cleanup

* Complementary fix for #213. Pass body to worker_name for calls from AutoExtendVisibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@takeyuweb @phstc @mariokostelac @houndci-bot