Replace manual connection control with abstracted execution wrapper #1746
Conversation
As described in https://github.com/rails/rails/blob/1d9d53831600c974fcccd4b5574f0e03d7e1513c/guides/source/threading_and_code_execution.md#executor `Rails.application.executor.wrap` should provide a higher-level of abstraction to achieve concurrent execution, which includes returning acquired ActiveRecord connection. This prevents us from locking in a particular ORM's database connection mechanism.
| @@ -2,7 +2,7 @@ module Sentry | |||
| class BackgroundWorker | |||
| def _perform(&block) | |||
| # make sure the background worker returns AR connection if it accidentally acquire one during serialization | |||
| ActiveRecord::Base.connection_pool.with_connection do | |||
| ::Rails.application.executor.wrap do | |||
There was a problem hiding this comment.
After Rails 7.0, it'll capture the block's exception and report it, which will enter here again and cause infinite loop. So now I think this may be a bad idea 😕
There was a problem hiding this comment.
but our @__sentry_captured fix will avoid the loop right?
There was a problem hiding this comment.
Not, because the loop will be caused by the exception from SDK (if there's any), instead of the one we capture.
For example, if an event for ZeroDivisionError caused a NoMethodError during serialization, that NoMethodError will be captured and became a new event. If that NoMethodError is somehow a problem of the SDK code and/or problematic contextual data, it'll be an infinite loop.
There was a problem hiding this comment.
right but then we can just add another rescue here, SDK should fail gracefully internally.
Or move this rescue over to both _perform methods.
There was a problem hiding this comment.
Yeah but that feels ugly. And if this doesn't actually solve #1745, I don't see the necessity to implement it like this.
There was a problem hiding this comment.
ah ok just read the follow up in the other issue
As described in this document,
Rails.application.executor.wrapshould provide a higher-level of abstraction to achieve concurrent execution, which includes returning acquired ActiveRecord connection.This prevents us from locking in a particular ORM's database connection mechanism.