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

ActiveSupport expects a hash as a payload for notifications #20

Merged
merged 3 commits into from
Sep 15, 2015
Merged

ActiveSupport expects a hash as a payload for notifications #20

merged 3 commits into from
Sep 15, 2015

Conversation

workmad3
Copy link
Contributor

ActiveSupport expects a hash to be passed as the payload to instrument. In the majority of cases, not passing a hash doesn't cause any incorrect behaviour.

However, if the block passed to instrument raises an Exception, ActiveSupport::Notifications will attempt to add an :exception key to the payload hash. When this occurs within a cache fetch for ReadThis, the end result is a TypeError from deep inside the cache code, unrelated to the original exception. (https://github.com/rails/rails/blob/30af171af13293aa37bee612ae7b976d3ede0439/activesupport/lib/active_support/notifications.rb#L64)

(credit goes to @banister for figuring out this edge case)

@sorentwo
Copy link
Owner

Awfully the spec for notifications passes a hash rather than the single key. Unfortunately the instrument block itself isn't tested by the cache spec, so this wasn't caught. Thanks for the patch, it's appreciated.

Will you add an entry to the CHANGELOG and give yourself credit please?

@workmad3
Copy link
Contributor Author

No worries, we just happened to discover this when figuring out problems with a mis-behaving redis instance, and @banister took the time to dig into the odd TypeError exceptions we were getting.

Hopefully the CHANGELOG entry is ok.

Forgot to add the [pull-20] link reference.
@sorentwo
Copy link
Owner

Looks great. Perfect.

sorentwo added a commit that referenced this pull request Sep 15, 2015
…-payload

ActiveSupport expects a hash as a payload for notifications
@sorentwo sorentwo merged commit e490fb2 into sorentwo:master Sep 15, 2015
@workmad3 workmad3 deleted the use-hash-for-notification-payload branch September 15, 2015 14:56
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

Successfully merging this pull request may close these issues.

None yet

2 participants