search

sorentwo / readthis

:newspaper: Pooled active support compliant caching with redis
MIT License
504 stars 40 forks source link

ActiveSupport expects a hash as a payload for notifications #20

Closed workmad3 closed 9 years ago

workmad3 commented 9 years ago

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

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

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.

sorentwo commented 9 years ago

Looks great. Perfect.