diff options
Diffstat (limited to 'lib/feature.rb')
-rw-r--r-- | lib/feature.rb | 99 |
1 files changed, 71 insertions, 28 deletions
diff --git a/lib/feature.rb b/lib/feature.rb index 47fee23c7ea..b5a97ee8f9b 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -42,8 +42,10 @@ class Feature flipper.features.to_a end + RecursionError = Class.new(RuntimeError) + def get(key) - flipper.feature(key) + with_feature(key, &:itself) end def persisted_names @@ -65,34 +67,29 @@ class Feature persisted_names.include?(feature_name.to_s) end - # use `default_enabled: true` to default the flag to being `enabled` - # unless set explicitly. The default is `disabled` - # TODO: remove the `default_enabled:` and read it from the `definition_yaml` - # check: https://gitlab.com/gitlab-org/gitlab/-/issues/30228 - def enabled?(key, thing = nil, type: :development, default_enabled: false) + # The default state of feature flag is read from `YAML`: + # 1. If feature flag does not have YAML it will fallback to `default_enabled: false` + # in production environment, but raise exception in development or tests. + # 2. The `default_enabled_if_undefined:` is tech debt related to Gitaly flags + # and should not be used outside of Gitaly's `lib/feature/gitaly.rb` + def enabled?(key, thing = nil, type: :development, default_enabled_if_undefined: nil) if check_feature_flags_definition? if thing && !thing.respond_to?(:flipper_id) raise InvalidFeatureFlagError, "The thing '#{thing.class.name}' for feature flag '#{key}' needs to include `FeatureGate` or implement `flipper_id`" end - Feature::Definition.valid_usage!(key, type: type, default_enabled: default_enabled) + Feature::Definition.valid_usage!(key, type: type) end - # If `default_enabled: :yaml` we fetch the value from the YAML definition instead. - default_enabled = Feature::Definition.default_enabled?(key) if default_enabled == :yaml - - # During setup the database does not exist yet. So we haven't stored a value - # for the feature yet and return the default. - return default_enabled unless ApplicationRecord.database.exists? + default_enabled = Feature::Definition.default_enabled?(key, default_enabled_if_undefined: default_enabled_if_undefined) - feature = get(key) + feature_value = with_feature(key) do |feature| + feature_value = current_feature_value(feature, thing, default_enabled: default_enabled) + end - # If we're not default enabling the flag or the feature has been set, always evaluate. - # `persisted?` can potentially generate DB queries and also checks for inclusion - # in an array of feature names (177 at last count), possibly reducing performance by half. - # So we only perform the `persisted` check if `default_enabled: true` - feature_value = !default_enabled || Feature.persisted_name?(feature.name) ? feature.enabled?(thing) : true + # If not yielded, then either recursion is happening, or the database does not exist yet, so use default_enabled. + feature_value = default_enabled if feature_value.nil? # If we don't filter out this flag here we will enter an infinite loop log_feature_flag_state(key, feature_value) if log_feature_flag_states?(key) @@ -100,46 +97,46 @@ class Feature feature_value end - def disabled?(key, thing = nil, type: :development, default_enabled: false) + def disabled?(key, thing = nil, type: :development, default_enabled_if_undefined: nil) # we need to make different method calls to make it easy to mock / define expectations in test mode - thing.nil? ? !enabled?(key, type: type, default_enabled: default_enabled) : !enabled?(key, thing, type: type, default_enabled: default_enabled) + thing.nil? ? !enabled?(key, type: type, default_enabled_if_undefined: default_enabled_if_undefined) : !enabled?(key, thing, type: type, default_enabled_if_undefined: default_enabled_if_undefined) end def enable(key, thing = true) log(key: key, action: __method__, thing: thing) - get(key).enable(thing) + with_feature(key) { _1.enable(thing) } end def disable(key, thing = false) log(key: key, action: __method__, thing: thing) - get(key).disable(thing) + with_feature(key) { _1.disable(thing) } end def enable_percentage_of_time(key, percentage) log(key: key, action: __method__, percentage: percentage) - get(key).enable_percentage_of_time(percentage) + with_feature(key) { _1.enable_percentage_of_time(percentage) } end def disable_percentage_of_time(key) log(key: key, action: __method__) - get(key).disable_percentage_of_time + with_feature(key, &:disable_percentage_of_time) end def enable_percentage_of_actors(key, percentage) log(key: key, action: __method__, percentage: percentage) - get(key).enable_percentage_of_actors(percentage) + with_feature(key) { _1.enable_percentage_of_actors(percentage) } end def disable_percentage_of_actors(key) log(key: key, action: __method__) - get(key).disable_percentage_of_actors + with_feature(key, &:disable_percentage_of_actors) end def remove(key) return unless persisted_name?(key) log(key: key, action: __method__) - get(key).remove + with_feature(key, &:remove) end def reset @@ -181,6 +178,52 @@ class Feature private + # Evaluate if `default enabled: false` or the feature has been persisted. + # `persisted_name?` can potentially generate DB queries and also checks for inclusion + # in an array of feature names (177 at last count), possibly reducing performance by half. + # So we only perform the `persisted` check if `default_enabled: true` + def current_feature_value(feature, thing, default_enabled:) + return true if default_enabled && !Feature.persisted_name?(feature.name) + + feature.enabled?(thing) + end + + # NOTE: it is not safe to call `Flipper::Feature#enabled?` outside the block + def with_feature(key) + feature = unsafe_get(key) + yield feature if feature.present? + ensure + pop_recursion_stack + end + + def unsafe_get(key) + # During setup the database does not exist yet. So we haven't stored a value + # for the feature yet and return the default. + return unless ApplicationRecord.database.exists? + + flag_stack = ::Thread.current[:feature_flag_recursion_check] || [] + Thread.current[:feature_flag_recursion_check] = flag_stack + + # Prevent more than 10 levels of recursion. This limit was chosen as a fairly + # low limit while allowing some nesting of flag evaluation. We have not seen + # this limit hit in production. + if flag_stack.size > 10 + Gitlab::ErrorTracking.track_exception(RecursionError.new('deep recursion'), stack: flag_stack) + return + elsif flag_stack.include?(key) + Gitlab::ErrorTracking.track_exception(RecursionError.new('self recursion'), stack: flag_stack) + return + end + + flag_stack.push(key) + flipper.feature(key) + end + + def pop_recursion_stack + flag_stack = Thread.current[:feature_flag_recursion_check] + flag_stack.pop if flag_stack + end + def flipper if Gitlab::SafeRequestStore.active? Gitlab::SafeRequestStore[:flipper] ||= build_flipper_instance(memoize: true) |