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

Add default recovery window option to acts as paranoid #4

Open
wants to merge 12 commits into
base: core
Choose a base branch
from

Conversation

nathanbertram
Copy link

@nathanbertram nathanbertram commented Aug 23, 2017

Background
Procure is currently running this version of this gem rubysherpas#170 which adds support for a :dependent_recovery_window option to be set at the model level with acts_as_paranoia dependent_recovery_window: 10.minutes . This pull request was not accepted at the time from the maintainer. However, since then a new maintainer has let a similar PR merge into the main core branch rubysherpas#383 pass. However, what we need is the ability to set this a default at the model and application level to preserve existing functionality in the app.

What the recovery window option does
When recovering records recursively, associated records will only be restored if they were deleted within the recovery window timespan of the parent's deletion time. This is to enable recovery of a record's associations as they existed at the time it was destroyed.

What has changed
procore/paranoia:core has been synced with rubysherpas/paranoia:core to add Rails 5 support. This branch has been created off of this.

This PR adds that default :dependent_recovery_window option. However, to be consistent with the up to date repo this has been renamed simply :recovery_window

Copy link
Member

@aks aks left a comment

Choose a reason for hiding this comment

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

Avoid class variables for any ruby code that can ever be used in a multithreaded environment (e.g., sidekiq)

@@ -2,6 +2,7 @@

module Paranoia
@@default_sentinel_value = nil
@@default_recovery_window = nil
Copy link
Member

Choose a reason for hiding this comment

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

Does lib/paranoia.rb ever get run in a thread? If so, the use of class variables will not work. Each thread can independently set the class variable, but they all share the same class variable.

In a multithreaded environment, such as in sidekiq, use thread-local variables:

    Thread.current[:default_sentinel_value] = nil
    Thread.current[:default_recovery_window] = nil

Copy link

@sudara sudara Oct 30, 2017

Choose a reason for hiding this comment

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

Hmm, I understand this concern. My thoughts:

  1. This is following convention in the existing gem (class var getter/setter) https://github.com/rubysherpas/paranoia/blob/core/lib/paranoia.rb

  2. We are hoping to get this merged upstream (hence wanting to follow their convention)

  3. The class variables are only ever set in a rails initializer.

In practice these variables are more than thread safe — they are just "hooks" to give gem users the ability to set pseudo-constants for the gem once, when the app loads.

But in theory, you are 100% right, it is not thread-safe if someone chose to dynamically change these values. I personally can't imagine it ever being an issue. If you feel strongly about it we could re-strategize about what to do re: upstream.

@@ -12,6 +13,14 @@ def self.default_sentinel_value
@@default_sentinel_value
end

def self.default_recovery_window=(val)
@@default_recovery_window = val
Copy link
Member

Choose a reason for hiding this comment

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

This won't work in a multithreaded environment. Use thread-local variables:

    Thread.current[:default_recovery_window] = val

end

def self.default_recovery_window
@@default_recovery_window
Copy link
Member

Choose a reason for hiding this comment

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

Use thread-local variables to enable usage in threaded code (eg: within a sidekiq job):

    Thread.current[:default_recovery_window]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants