Merge pull request #52 from viralpraxis/implement-rack-middleware-ins…

…tance-variable-cop

Implement `ThreadSafety/RackMiddlewareInstanceVariable` cop

.rubocop.yml

@@ -81,6 +81,8 @@ Style/SymbolArray:
 
 RSpec/ExampleLength:
   Max: 11
+  Exclude:
+    - spec/rubocop/cop/thread_safety/rack_middleware_instance_variable_spec.rb
 
 RSpec/ContextWording:
   Exclude:

CHANGELOG.md

@@ -2,7 +2,8 @@
 
 ## master (unreleased)
 
-* [#52](https://github.com/rubocop/rubocop-thread_safety/pull/52): Drop support for RuboCop older than 1.48. ([@viralpraxis](https://github.com/viralpraxis))
+* [#54](https://github.com/rubocop/rubocop-thread_safety/pull/54): Drop support for RuboCop older than 1.48. ([@viralpraxis](https://github.com/viralpraxis))
+* [#52](https://github.com/rubocop/rubocop-thread_safety/pull/52): Add new `RackMiddlewareInstanceVariable` cop to detect instance variables in Rack middleware. ([@viralpraxis](https://github.com/viralpraxis))
 * [#48](https://github.com/rubocop/rubocop-thread_safety/pull/48): Do not report instance variables in `ActionDispatch` callbacks in singleton methods. ([@viralpraxis](https://github.com/viralpraxis))
 * [#43](https://github.com/rubocop/rubocop-thread_safety/pull/43): Make detection of ActiveSupport's `class_attribute` configurable. ([@viralpraxis](https://github.com/viralpraxis))
 * [#42](https://github.com/rubocop/rubocop-thread_safety/pull/42): Fix some `InstanceVariableInClassMethod` cop false positive offenses. ([@viralpraxis](https://github.com/viralpraxis))

config/default.yml

@@ -35,3 +35,13 @@ ThreadSafety/NewThread:
 ThreadSafety/DirChdir:
   Description: Avoid using `Dir.chdir` due to its process-wide effect.
   Enabled: true
+
+ThreadSafety/RackMiddlewareInstanceVariable:
+  Description: Avoid instance variables in Rack middleware.
+  Enabled: true
+  Include:
+    - 'app/middleware/**/*.rb'
+    - 'lib/middleware/**/*.rb'
+    - 'app/middlewares/**/*.rb'
+    - 'lib/middlewares/**/*.rb'
+  AllowedIdentifiers: []

docs/modules/ROOT/pages/cops.adoc

@@ -5,3 +5,4 @@
 * xref:cops_threadsafety.adoc#threadsafetymutableclassinstancevariable[ThreadSafety/MutableClassInstanceVariable]
 * xref:cops_threadsafety.adoc#threadsafetynewthread[ThreadSafety/NewThread]
 * xref:cops_threadsafety.adoc#threadsafetydirchdir[ThreadSafety/DirChdir]
+* xref:cops_threadsafety.adoc#threadsafetyrackmiddlewareinstancevariable[ThreadSafety/RackMiddlewareInstanceVariable]

docs/modules/ROOT/pages/cops_threadsafety.adoc

@@ -267,3 +267,78 @@ Let a framework like Sidekiq handle the threads.
 # bad
 Thread.new { do_work }
 ----
+
+== ThreadSafety/RackMiddlewareInstanceVariable
+
+|===
+| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed
+
+| Enabled
+| Yes
+| No
+| -
+| -
+|===
+
+Avoid instance variables in rack middleware.
+
+Middlewares are initialized once, meaning any instance variables are shared between executor threads.
+To avoid potential race conditions, it's recommended to design middlewares to be stateless
+or to implement proper synchronization mechanisms.
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+class CounterMiddleware
+  def initialize(app)
+    @app = app
+    @counter = 0
+  end
+
+  def call(env)
+    app.call(env)
+  ensure
+    @counter += 1
+  end
+end
+
+# good
+class CounterMiddleware
+  def initialize(app)
+    @app = app
+    @counter = Concurrent::AtomicReference.new(0)
+  end
+
+  def call(env)
+    app.call(env)
+  ensure
+    @counter.update { |ref| ref + 1 }
+  end
+end
+
+class IdentityMiddleware
+  def initialize(app)
+    @app = app
+  end
+
+  def call(env)
+    app.call(env)
+  end
+end
+----
+
+=== Configurable attributes
+
+|===
+| Name | Default value | Configurable values
+
+| Include
+| `+app/middleware/**/*.rb+`, `+lib/middleware/**/*.rb+`, `+app/middlewares/**/*.rb+`, `+lib/middlewares/**/*.rb+`
+| Array
+
+| AllowedIdentifiers
+| `[]`
+| Array
+|===

lib/rubocop-thread_safety.rb

@@ -8,8 +8,11 @@
 
 RuboCop::ThreadSafety::Inject.defaults!
 
+require 'rubocop/cop/mixin/operation_with_threadsafe_result'
+
 require 'rubocop/cop/thread_safety/instance_variable_in_class_method'
 require 'rubocop/cop/thread_safety/class_and_module_attributes'
 require 'rubocop/cop/thread_safety/mutable_class_instance_variable'
 require 'rubocop/cop/thread_safety/new_thread'
 require 'rubocop/cop/thread_safety/dir_chdir'
+require 'rubocop/cop/thread_safety/rack_middleware_instance_variable'

lib/rubocop/cop/mixin/operation_with_threadsafe_result.rb

@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    # Common functionality for checking if a well-known operation
+    # produces an object with thread-safe semantics.
+    module OperationWithThreadsafeResult
+      extend NodePattern::Macros
+
+      # @!method operation_produces_threadsafe_object?(node)
+      def_node_matcher :operation_produces_threadsafe_object?, <<~PATTERN
+        {
+          (send (const {nil? cbase} :Queue) :new ...)
+          (send
+            (const (const {nil? cbase} :ThreadSafe) {:Hash :Array})
+            :new ...)
+          (block
+            (send
+              (const (const {nil? cbase} :ThreadSafe) {:Hash :Array})
+              :new ...)
+            ...)
+          (send (const (const {nil? cbase} :Concurrent) _) :new ...)
+          (block
+            (send (const (const {nil? cbase} :Concurrent) _) :new ...)
+            ...)
+          (send (const (const (const {nil? cbase} :Concurrent) _) _) :new ...)
+          (block
+            (send
+              (const (const (const {nil? cbase} :Concurrent) _) _)
+              :new ...)
+            ...)
+          (send
+            (const (const (const (const {nil? cbase} :Concurrent) _) _) _)
+            :new ...)
+          (block
+            (send
+              (const (const (const (const {nil? cbase} :Concurrent) _) _) _)
+              :new ...)
+            ...)
+        }
+      PATTERN
+    end
+  end
+end

lib/rubocop/cop/thread_safety/mutable_class_instance_variable.rb

@@ -74,8 +74,10 @@ module ThreadSafety
       #   end
       class MutableClassInstanceVariable < Base
         extend AutoCorrector
+
         include FrozenStringLiteral
         include ConfigurableEnforcedStyle
+        include OperationWithThreadsafeResult
 
         MSG = 'Freeze mutable objects assigned to class instance variables.'
         FROZEN_STRING_LITERAL_TYPES_RUBY27 = %i[str dstr].freeze
@@ -241,39 +243,6 @@ def correct_splat_expansion(corrector, expr, splat_value)
           }
         PATTERN
 
-        # @!method operation_produces_threadsafe_object?(node)
-        def_node_matcher :operation_produces_threadsafe_object?, <<~PATTERN
-          {
-            (send (const {nil? cbase} :Queue) :new ...)
-            (send
-              (const (const {nil? cbase} :ThreadSafe) {:Hash :Array})
-              :new ...)
-            (block
-              (send
-                (const (const {nil? cbase} :ThreadSafe) {:Hash :Array})
-                :new ...)
-              ...)
-            (send (const (const {nil? cbase} :Concurrent) _) :new ...)
-            (block
-              (send (const (const {nil? cbase} :Concurrent) _) :new ...)
-              ...)
-            (send (const (const (const {nil? cbase} :Concurrent) _) _) :new ...)
-            (block
-              (send
-                (const (const (const {nil? cbase} :Concurrent) _) _)
-                :new ...)
-              ...)
-            (send
-              (const (const (const (const {nil? cbase} :Concurrent) _) _) _)
-              :new ...)
-            (block
-              (send
-                (const (const (const (const {nil? cbase} :Concurrent) _) _) _)
-                :new ...)
-              ...)
-          }
-        PATTERN
-
         # @!method range_enclosed_in_parentheses?(node)
         def_node_matcher :range_enclosed_in_parentheses?, <<~PATTERN
           (begin ({irange erange} _ _))

lib/rubocop/cop/thread_safety/rack_middleware_instance_variable.rb

@@ -0,0 +1,120 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module ThreadSafety
+      # Avoid instance variables in rack middleware.
+      #
+      # Middlewares are initialized once, meaning any instance variables are shared between executor threads.
+      # To avoid potential race conditions, it's recommended to design middlewares to be stateless
+      # or to implement proper synchronization mechanisms.
+      #
+      # @example
+      #   # bad
+      #   class CounterMiddleware
+      #     def initialize(app)
+      #       @app = app
+      #       @counter = 0
+      #     end
+      #
+      #     def call(env)
+      #       app.call(env)
+      #     ensure
+      #       @counter += 1
+      #     end
+      #   end
+      #
+      #   # good
+      #   class CounterMiddleware
+      #     def initialize(app)
+      #       @app = app
+      #       @counter = Concurrent::AtomicReference.new(0)
+      #     end
+      #
+      #     def call(env)
+      #       app.call(env)
+      #     ensure
+      #       @counter.update { |ref| ref + 1 }
+      #     end
+      #   end
+      #
+      #   class IdentityMiddleware
+      #     def initialize(app)
+      #       @app = app
+      #     end
+      #
+      #     def call(env)
+      #       app.call(env)
+      #     end
+      #   end
+      class RackMiddlewareInstanceVariable < Base
+        include AllowedIdentifiers
+        include OperationWithThreadsafeResult
+
+        MSG = 'Avoid instance variables in Rack middleware.'
+
+        RESTRICT_ON_SEND = %i[instance_variable_get instance_variable_set].freeze
+
+        # @!method rack_middleware_like_class?(node)
+        def_node_matcher :rack_middleware_like_class?, <<~MATCHER
+          (class (const nil? _) nil? (begin <(def :initialize (args (arg _)+) ...) (def :call (args (arg _)) ...) ...>))
+        MATCHER
+
+        # @!method app_variable(node)
+        def_node_search :app_variable, <<~MATCHER
+          (def :initialize (args (arg $_) ...) `(ivasgn $_ (lvar $_)))
+        MATCHER
+
+        def on_class(node)
+          return unless rack_middleware_like_class?(node)
+
+          constructor_method = find_constructor_method(node)
+          return unless (application_variable = extract_application_variable_from_contructor_method(constructor_method))
+
+          safe_variables = extract_safe_variables_from_constructor_method(constructor_method)
+
+          node.each_node(:def) do |def_node|
+            def_node.each_node(:ivasgn, :ivar) do |ivar_node|
+              variable, = ivar_node.to_a
+              if variable == application_variable || safe_variables.include?(variable) || allowed_identifier?(variable)
+                next
+              end
+
+              add_offense ivar_node
+            end
+          end
+        end
+
+        def on_send(node)
+          argument = node.first_argument
+
+          return unless argument&.sym_type? || argument&.str_type?
+          return if allowed_identifier?(argument.value)
+
+          add_offense node
+        end
+
+        private
+
+        def find_constructor_method(class_node)
+          class_node
+            .each_node(:def)
+            .find { |node| node.method?(:initialize) && node.arguments.size >= 1 }
+        end
+
+        def extract_application_variable_from_contructor_method(constructor_method)
+          constructor_method
+            .then { |node| app_variable(node) }
+            .then { |variables| variables.first[1] if variables.first }
+        end
+
+        def extract_safe_variables_from_constructor_method(constructor_method)
+          constructor_method
+            .each_node(:ivasgn)
+            .select { |ivasgn_node| operation_produces_threadsafe_object?(ivasgn_node.to_a[1]) }
+            .map { _1.to_a[0] }
+        end
+      end
+    end
+  end
+end