covermymeds/rubocop-thread_safety

RuboCop::ThreadSafety

Notice of official fork

This project is now part of the RuboCop organization at rubocop/rubocop-thread_safety. Please follow development there.

---

Thread-safety analysis for your projects, as an extension to RuboCop.

Installation and Usage

Installation into an application

Add this line to your application's Gemfile (using require: false as it's a standalone tool):

gem 'rubocop-thread_safety', require: false

Install it with Bundler by invoking:

$ bundle

Add this line to your application's .rubocop.yml:

require: rubocop-thread_safety

Now you can run rubocop and it will automatically load the RuboCop Thread-Safety cops together with the standard cops.

Scanning an application without adding it to the Gemfile

Install the gem:

$ gem install rubocop-thread_safety

Scan the application for just thread-safety issues:

$ rubocop -r rubocop-thread_safety --only ThreadSafety,Style/GlobalVars,Style/ClassVars,Style/MutableConstant

Configuration

There are some added configuration options that can be tweaked to modify the behaviour of these thread-safety cops.

Correcting code for thread-safety

There are a few ways to improve thread-safety that stem around avoiding unsynchronized mutation of state that is shared between multiple threads.

State shared between threads may take various forms, including:

• Class variables (@@name). Note: these affect child classes too.
• Class instance variables (@name in class context or class methods)
• Constants (NAME). Ruby will warn if a constant is re-assigned to a new value but will allow it. Mutable objects can still be mutated (e.g. push to an array) even if they are assigned to a constant.
• Globals ($name), with the possible exception of some special globals provided by ruby that are documented as thread-local like regular expression results.
• Variables in the scope of created threads (where Thread.new is called).

Improvements that would make shared state thread-safe include:

• freeze objects to protect against mutation. Note: freeze is shallow, i.e. freezing an array will not also freeze its elements.
• Use data structures or concurrency abstractions from concurrent-ruby, e.g. Concurrent::Map
• Use a Mutex or similar to synchronize access.
• Use ActiveSupport::CurrentAttributes
• Use RequestStore
• Use Thread.current[:name]

Development

After checking out the repo, run bin/setup to install dependencies. Then, run rake spec to run the tests. You can also run bin/console for an interactive prompt that will allow you to experiment.

To install this gem onto your local machine, run bundle exec rake install. To release a new version, update the version number in version.rb, and then run bundle exec rake release, which will create a git tag for the version, push git commits and tags, and push the .gem file to rubygems.org.

Contributing

Bug reports and pull requests are welcome on GitHub at https://github.com/covermymeds/rubocop-thread_safety.

Copyright

Copyright (c) 2016-2022 CoverMyMeds. See LICENSE.txt for further details.

rubocop-thread_safety's Issues

Show how to make it better as well

The comments of ghe cops show what kind of prectice is considered bad.

To improve it is alao relevant to understand what ia being considered the better practice.

Could you add examples or links to show how to do better in the comments?

Be smarter about Mutex use

Using:

SEMAPHORE = Mutex.new

def self.cool_class_method
  SEMAPHORE.synchronize do
    @sweet_sweet_class_ivar = 'radical'
  end
end

should let us safely set a class instance variable. If there is any way to detect assignment inside a Mutex instance's synchronize block and not throw a warning that would be 🏀 er

Disallow ivars in instance methods in modules named "ClassMethods"

Some class is almost certainly being extended with ClassMethods.

Consider disallowing ivars in methods of any module

As an extension to the suggestion in #4, perhaps we should flag up ivars used in methods of any module.

This would unfortunately add some false positives as modules may be included in classes, making instance methods and hence would be using instance variables rather than class instance variables.

They could be individually checked by the user and ignored with a rubocop:disable comment. Or perhaps this should be an option to the InstanceVariableInClassMethod, which may be set depending on how prevalent extending modules is in the user's codebase.

Concerns I would like to catch would be:

module Mod
  def some_method(something)
    @params = something
  end
end

module ExtMod
  extend Mod
end

class ExtClass
  extend Mod
end

ExtMod.some_method(something) # sets a class instance variable on ExtMod
ExtClass.some_method(something) # sets a class instance variable on ExtClass

False positives that would be difficult / impossible to rule out in a static analysis:

module Mod
  def some_method(something)
    @params = something
  end
end

class Includer
  include Mod
end

instance = Includer.new
instance.some_method(something) # sets an instance variable on instance

Enforce frozen class instance variables

Right now ClassAndModuleAttributes forbids attr_writer and attr_accessor on class instance variables. We should probably take this a step further and enforce frozen instance variables in this case (even if ruby doesn't have deep freezing).

The following code passes all the cops right now, even if its intended use is the same as an attr_accessor:

class MyClass
  @my_variable = 'class instance variable'

  class << self
    attr_reader :my_variable
  end

  def self.my_method
    my_variable << ' modified'
  end
end

MyClass.my_method # => "class instance variable modified"

What do you think?

False positives on inheritance

edit: realized I have opened in the wrong repo

False Positive: ivar in dynamic method definition of class.new

Code:

module QueryExtensions
  def self.ensure_where_not_chain(base)
    base.class_eval do
      const_get(:WhereNotChain)
    rescue NameError
      klass = Class.new {
        def initialize(scope)
          @scope = scope
        end
      }

      const_set(:WhereNotChain, klass)
    end
    base.const_get(:WhereNotChain)
  end
end

Results:

config/hooks/after_gems/03.query_extensions.rb:183:13: C: ThreadSafety/InstanceVariableInClassMethod: Avoid instance variables in class methods.
            @scope = scope
            ^^^^^^

The parser is detecting the method definition in the dynamically created class as having an instance variable in a class method. this is not the case. it is defining a method in the class and the instance variable is for an instance

Release New Version with Fix for ThreadSafety/NewThread Enabled option

Currently you cannot use the gem unless ThreadSafety/NewThread is enabled.

Can you please release a new version with the fix that allows Enabled: false to be set

Detect class_eval as class context

When checking for class / module context, detect class_eval, module_eval, class_exec, module_exec. The class_* variants are aliases of the module_* variants. It should however be safe to assume class_* would be called on a class and module_* would be called on a module.

I don't think it is worth looking at class_eval or module_eval when given a string argument. Just interested in when any of these are given a block.

A reduced example that inspired this thought:

class Example
  class << self
    attr_reader :separator
  end
end

def separate_with(separator)
  Example.class_eval do
    @separator = separator
  end
end

Given there are multiple cops that need to check for the first class / module context, I think it would be useful to create a mixin module to help with the search. Something like

module RuboCop
  module Cop
    module ThreadSafety
      module Mixin
        module ClassModuleContext
          # return first class or module context in node's ancestors
          def find_context(node)
            # return first node that is a class, module, class_eval, module_eval or similar
          end

          def class_context?(node)
            # return true if node is a class context
          end

          def module_context?(node)
            # return true if node is a module context
          end
        end
      end
    end
  end
end

Test against multiple versions of rubocop

The gemspec currently specifies rubocop >= 0.51.0.

To reduce problems for users that are not yet on the latest rubocop (myself included), it would be good to test against multiple versions of rubocop.

I'm currently working on adding an Appraisal. From this, it looks like rubocop < 0.53.0 are difficult to get working correctly with the new ThreadSafety/MutableClassInstanceVariable cop.

Proposal

1. Set minimum required rubocop version to 0.53.0 or above.
2. Add Appraisals to test at least an earliest and latest supported version of rubocop.

Clarify `class_attribute` thread safety issue

I can't figure out why class_attribute :attr is reported as thread-unsafe. Could someone please clarify if it's actually unsafe?

Here's how I smoke-test for safety:

class F
  class_attribute :foo
end

1000.times.map { |i| Thread.new { sleep(0.1); F.foo = i; raise unless F.foo == i; } }.each(&:join);

thanks!

license_spec fails

spec/license_spec.rb fails.

It seems to be solved by updating LICENSE.txt to include year 2022, as in e33eec3 .

$ ruby -v
ruby 3.1.0p0 (2021-12-25 revision fb4df44d16) [x86_64-darwin20]

$ bundle exec rake spec 
Randomized with seed 46857
...............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................FF...................

Failures:

  1) the LICENSE is referenced from the README
     Failure/Error:
       expect(readme).to match(
         /Copyright .* 2016-#{Date.today.year}.*LICENSE.txt/m
       )
     
       expected "# RuboCop::ThreadSafety\n\nThread-safety analysis for your projects, as an extension to\n[RuboCop](h...ight\n\nCopyright (c) 2016-2021 CoverMyMeds.\nSee [LICENSE.txt](LICENSE.txt) for further details.\n" to match /Copyright .* 2016-2022.*LICENSE.txt/m
       Diff:
       @@ -1,76 +1,151 @@
       -/Copyright .* 2016-2022.*LICENSE.txt/m
       +# RuboCop::ThreadSafety
       +
       +Thread-safety analysis for your projects, as an extension to
       +[RuboCop](https://github.com/bbatsov/rubocop).
       +
       +## Installation and Usage
       +
       +### Installation into an application
       +
       +Add this line to your application's Gemfile (using `require: false` as it's a standalone tool):
       +
       +```ruby
       +gem 'rubocop-thread_safety', require: false
       +```
       +
       +Install it with Bundler by invoking:
       +
       +    $ bundle
       +
       +Add this line to your application's `.rubocop.yml`:
       +
       +    require: rubocop-thread_safety
       +
       +Now you can run `rubocop` and it will automatically load the RuboCop
       +Thread-Safety cops together with the standard cops.
       +
       +### Scanning an application without adding it to the Gemfile
       +
       +Install the gem:
       +
       +    $ gem install rubocop-thread_safety
       +
       +Scan the application for just thread-safety issues:
       +
       +    $ rubocop -r rubocop-thread_safety --only ThreadSafety,Style/GlobalVars,Style/ClassVars,Style/MutableConstant
       +
       +### Configuration
       +
       +There are some added [configuration options](https://github.com/covermymeds/rubocop-thread_safety/blob/master/config/default.yml) that can be tweaked to modify the behaviour of these thread-safety cops.
       +
       +### Correcting code for thread-safety
       +
       +There are a few ways to improve thread-safety that stem around avoiding
       +unsynchronized mutation of state that is shared between multiple threads.
       +
       +State shared between threads may take various forms, including:
       +
       +* Class variables (`@@name`). Note: these affect child classes too.
       +* Class instance variables (`@name` in class context or class methods)
       +* Constants (`NAME`). Ruby will warn if a constant is re-assigned to a new value but will allow it. Mutable objects can still be mutated (e.g. push to an array) even if they are assigned to a constant.
       +* Globals (`$name`), with the possible exception of some special globals provided by ruby that are documented as thread-local like regular expression results.
       +* Variables in the scope of created threads (where `Thread.new` is called).
       +
       +Improvements that would make shared state thread-safe include:
       +
       +* `freeze` objects to protect against mutation. Note: `freeze` is shallow, i.e. freezing an array will not also freeze its elements.
       +* Use data structures or concurrency abstractions from [concurrent-ruby](https://github.com/ruby-concurrency/concurrent-ruby), e.g. `Concurrent::Map`
       +* Use a `Mutex` or similar to `synchronize` access.
       +* Use [`ActiveSupport::CurrentAttributes`](https://api.rubyonrails.org/classes/ActiveSupport/CurrentAttributes.html)
       +* Use [`RequestStore`](https://github.com/steveklabnik/request_store)
       +* Use `Thread.current[:name]`
       +
       +## Development
       +
       +After checking out the repo, run `bin/setup` to install dependencies. Then, run `rake spec` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment.
       +
       +To install this gem onto your local machine, run `bundle exec rake install`. To release a new version, update the version number in `version.rb`, and then run `bundle exec rake release`, which will create a git tag for the version, push git commits and tags, and push the `.gem` file to [rubygems.org](https://rubygems.org).
       +
       +## Contributing
       +
       +Bug reports and pull requests are welcome on GitHub at https://github.com/covermymeds/rubocop-thread_safety.
       +
       +## Copyright
       +
       +Copyright (c) 2016-2021 CoverMyMeds.
       +See [LICENSE.txt](LICENSE.txt) for further details.
       
     # ./spec/license_spec.rb:18:in `block (2 levels) in <top (required)>'

  2) the LICENSE contains a copyright statement for the current year
     Failure/Error: expect(license.read).to match(/Copyright 2016-#{Date.today.year}/)
     
       expected "Copyright 2016-2021 CoverMyMeds\n\nPermission is hereby granted, free of charge, to any person obtai...ING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.\n" to match /Copyright 2016-2022/
       Diff:
       @@ -1,7 +1,13 @@
       -/Copyright 2016-2022/
       +Copyright 2016-2021 CoverMyMeds
       +
       +Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions:
       +
       +The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.
       +
       +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
       
     # ./spec/license_spec.rb:13:in `block (2 levels) in <top (required)>'

Finished in 1.8 seconds (files took 2.42 seconds to load)
532 examples, 2 failures

Failed examples:

rspec ./spec/license_spec.rb:16 # the LICENSE is referenced from the README
rspec ./spec/license_spec.rb:12 # the LICENSE contains a copyright statement for the current year

Randomized with seed 46857

Rubocop-0.51.0 deprecated arguments format

Getting lots of these warnings with rubocop-0.51.0:

/myproject/vendor/bundle/ruby/2.2.0/gems/rubocop-thread_safety-0.3.3/lib/rubocop/cop/thread_safety/instance_variable_in_class_method.rb:24
Warning: The usage of positional location, message, and severity
parameters to Cop#add_offense is deprecated.
Please use keyword arguments instead.

The positional arguments version of Cop#add_offense will be removed in
RuboCop 0.52

allow `instance_variable_set/get` in `define_method` context

Howdy! Your gem is so great!

Just adding false positive we see:
https://github.com/railsbridge/bridge_troll/blob/76d67961c341ead6481d3046f7ac6428bd94efc8/app/models/concerns/presence_tracking_boolean.rb#L31-L34

simplified example:

def self.foo
  define_method("foo") do
    instance_variable_get("@foo")
  end
end

My understanding is that the instance_variable_get above is actually executed within the instance context and shouldn't trigger rubocop.

❤️

False positive with before_action set in class method

No offense reported

before_action {
  @my_var = 'foo'
}

Offense reported

def self.add_my_before_action
  before_action {
    @my_var = 'foo'
  }
end
add_my_before_action

License?

Repository doesn't contain any information about license of project.

Rubocop >= 1.20.0 breaks with this gem

Hello 👋🏼 Thanks for your gem!

Unfortunately, when upgrading to rubocop 1.20.0, running rubocop breaks when including rubocop-thread_safety with this error message:

An error occurred while ThreadSafety/MutableClassInstanceVariable cop was inspecting {full filepath}

This is due to a change to the constant FROZEN_STRING_LITERAL_TYPES.

Your code relies on this constant here:

rubocop-thread_safety/lib/rubocop/cop/thread_safety/mutable_class_instance_variable.rb

Line 181 in 9d5fa78

FROZEN_STRING_LITERAL_TYPES.include?(node.type) &&

I believe you can simply delete your own helper method frozen_string_literal?(node) as the module FrozenStringLiteral defines this as intended.

Incompatible with rubocop-0.47.0

Just trying this out with the latest rubocop https://github.com/bbatsov/rubocop/releases/tag/v0.47.0 and getting a bunch of errors, maybe they've renamed some stuff and this plugin needs updating.

Support detecting offenders in `ActiveSupport::Concern#class_methods` blocks

Just recently upgraded to v0.4.2 and I noticed that currently the cop doesn't pick up offenders in class_methods do...end blocks

Cases using module ClassMethods works great

module Personable
  extend ActiveSupport::Concern

  module ClassMethods
    def age
      @age ||= 33 # ThreadSafety/InstanceVariableInClassMethod error will occur here appropriately
    end
  end
end

Rails also allows us to use class_methods to achieve the same thing

module Personable
  extend ActiveSupport::Concern

  class_methods do
    def age
      @age ||= 33 # ThreadSafety/InstanceVariableInClassMethod won't detect this
    end
  end
end

It'd be awesome if we could detect cases inside class_methods blocks as well 🤩

https://api.rubyonrails.org/v6.1.3.2/classes/ActiveSupport/Concern.html#method-i-class_methods