isolate eval call so it can't muck with local variables

- update rubocop, and fix all rubocop failures
- Refactor specs

Author: nerdrew

.github/workflows/ruby.yml

@@ -18,7 +18,6 @@ permissions:
 
 jobs:
   test:
-
     runs-on: ubuntu-latest
     strategy:
       matrix:
@@ -27,10 +26,7 @@ jobs:
     steps:
     - uses: actions/checkout@v4
     - name: Set up Ruby
-    # To automatically get bug fixes and new Ruby versions for ruby/setup-ruby,
-    # change this to (see https://github.com/ruby/setup-ruby#versioning):
-    # uses: ruby/setup-ruby@v1
-      uses: ruby/setup-ruby@55283cc23133118229fd3f97f9336ee23a179fcf # v1.146.0
+      uses: ruby/setup-ruby@v1
       with:
         ruby-version: ${{ matrix.ruby-version }}
         bundler-cache: true # runs 'bundle install' and caches installed gems automatically

.rubocop.yml

@@ -1,15 +1,25 @@
 AllCops:
-  TargetRubyVersion: 2.4
+  TargetRubyVersion: 3.1
   DisplayCopNames: true
+  NewCops: enable
 
-Style/FrozenStringLiteralComment:
-  Enabled: true
+plugins:
+  - rubocop-rake
+  - rubocop-rspec
 
+Layout/LineLength:
+  Enabled: false
 Metrics:
   Enabled: false
-
+RSpec/DescribedClass:
+  Enabled: false
+RSpec/ExampleLength:
+  Enabled: false
+RSpec/MultipleExpectations:
+  Enabled: false
 Style/Documentation:
   Enabled: false
-
+Style/FrozenStringLiteralComment:
+  Enabled: true
 Style/StringLiterals:
   EnforcedStyle: double_quotes

Gemfile

@@ -5,7 +5,13 @@ source "https://rubygems.org"
 # Specify your gem's dependencies in debug_socket.gemspec
 gemspec
 
-gem "pry"
+gem "base64"
+gem "ostruct"
+gem "pry-byebug"
 gem "rake"
 gem "rspec", "~> 3.8"
-gem "rubocop", "0.59.1"
+gem "rubocop", "1.73.1"
+gem "rubocop-rake"
+gem "rubocop-rspec"
+gem "ruby-lsp", require: false
+gem "syntax_tree", require: false

debug_socket.gemspec

@@ -17,4 +17,7 @@ Gem::Specification.new do |spec|
   spec.bindir        = "exe"
   spec.executables   = spec.files.grep(%r{^exe/}) { |f| File.basename(f) }
   spec.require_paths = ["lib"]
+  spec.metadata["rubygems_mfa_required"] = "true"
+
+  spec.required_ruby_version = ">= 3.1"
 end

exe/debug-socket

@@ -12,8 +12,10 @@ end
 socket_path = ARGV[0]
 command = ARGV[1] || "backtrace"
 
-warn "\nSending `#{command}` to the following socket: #{socket_path}"\
+warn(
+  "\nSending `#{command}` to the following socket: #{socket_path}" \
   "----------------------------------------------------------\n\n"
+)
 
 UNIXSocket.open(socket_path) do |socket|
   socket.write(command)

lib/debug_socket.rb

@@ -16,7 +16,7 @@ def self.logger
     return @logger if defined?(@logger)
 
     require "logger"
-    @logger = Logger.new(STDERR)
+    @logger = Logger.new($stderr)
   end
 
   def self.start(path, &block)
@@ -32,22 +32,27 @@ def self.start(path, &block)
 
     server = UNIXServer.new(@path)
     @thread = Thread.new do
+      errors = 0
       loop do
-        begin
-          socket = server.accept
-          input = socket.read
-          logger&.warn("debug-socket-command=#{input.inspect}")
-
-          self.perform_audit(input, &block) if block
-
-          socket.puts(eval(input)) # rubocop:disable Security/Eval
-
-        rescue Exception => e # rubocop:disable Lint/RescueException
-          logger&.error { "debug-socket-error=#{e.inspect} backtrace=#{e.backtrace.inspect}" }
-        ensure
-          socket&.close
-        end
+        socket = server.accept
+        input = socket.read
+        logger&.warn("debug-socket-command=#{input.inspect}")
+
+        perform_audit(input, &block) if block
+        socket.puts(isolated_eval(input))
+
+        errors = 0
+      rescue StandardError => e
+        logger&.error { "debug-socket-error=#{e.inspect} errors=#{errors} path=#{@path} backtrace=#{e.backtrace.inspect}" }
+        errors += 1
+        raise e if errors > 20
+
+        sleep(1)
+      ensure
+        socket&.close
       end
+    rescue Exception => e # rubocop:disable Lint/RescueException
+      logger&.error { "debug-socket-error=#{e.inspect} DebugSocket is broken now path=#{@path} backtrace=#{e.backtrace.inspect}" }
     end
 
     logger&.unknown { "Debug socket listening on #{@path}" }
@@ -68,8 +73,7 @@ def self.stop
   def self.backtrace
     pid = Process.pid
     "#{Time.now.utc.iso8601} #{$PROGRAM_NAME}\n" + Thread.list.map do |thread|
-      output =
-        +"#{Time.now.utc.iso8601} pid=#{pid} thread.object_id=#{thread.object_id} thread.status=#{thread.status}"
+      output = "#{Time.now.utc.iso8601} pid=#{pid} thread.object_id=#{thread.object_id} thread.status=#{thread.status}"
       backtrace = thread.backtrace || []
       output << "\n#{backtrace.join("\n")}" if backtrace
       output
@@ -79,7 +83,14 @@ def self.backtrace
   # Allow debug socket input commands to be audited by an external callback
   private_class_method def self.perform_audit(input)
     yield input
-  rescue Exception => e
-    logger&.error "debug-socket-error=callback unsuccessful: #{e.inspect} for #{input.inspect} socket_path=#{@path}"
+  rescue Exception => e # rubocop:disable Lint/RescueException
+    logger&.error "debug-socket-error=callback unsuccessful: #{e.inspect} for #{input.inspect} path=#{@path} backtrace=#{e.backtrace.inspect}"
+  end
+
+  private_class_method def self.isolated_eval(input)
+    eval(input) # rubocop:disable Security/Eval
+  rescue Exception => e # rubocop:disable Lint/RescueException
+    logger&.error { "debug-socket-error=#{e.inspect} input=#{input.inspect} path=#{@path} backtrace=#{e.backtrace.inspect}" }
+    e.inspect
   end
 end

spec/debug_socket_spec.rb

@@ -22,16 +22,28 @@
       DebugSocket.start(path)
     end
 
+    def write(str, socket_path = path)
+      20.times { sleep(0.1) unless File.exist?(socket_path) }
+      socket = UNIXSocket.new(socket_path)
+
+      raise "Socket write timeout=#{socket_path}" unless socket.wait_writable(1)
+
+      socket.write(str)
+      socket.close_write
+
+      raise "Socket read timeout=#{socket_path}" unless socket.wait_readable(1)
+
+      socket.read
+    end
+
     after do
       DebugSocket.stop
       10.times { sleep(1) if File.exist?(path) }
       raise "did not cleanup socket file" if File.exist?(path)
     end
 
     it "logs and evals input" do
-      socket.write("2 + 2")
-      socket.close_write
-      expect(socket.read).to eq("4\n")
+      expect(write("2 + 2")).to eq("4\n")
       expect(log_buffer.string).to include('debug-socket-command="2 + 2"')
     end
 
@@ -55,38 +67,62 @@
           expect { DebugSocket.start(another_path) }
             .to raise_exception("debug socket thread already running for this process")
 
-          10.times { sleep(1) unless File.exist?(another_path) }
-          another_socket = UNIXSocket.new(another_path)
-          another_socket.write("Thread.list.each(&:wakeup)")
-          another_socket.close_write
-          expect(another_socket.read).to match(/Thread/)
+          expect(write("Thread.list.each(&:wakeup)", another_path)).to match(/Thread/)
           Process.wait(child, Process::WNOHANG)
         else
-          DebugSocket.start(another_path)
-          sleep
-          sleep(1)
-          DebugSocket.stop
-          exit!(1)
+          begin
+            DebugSocket.start(another_path)
+            sleep
+            sleep(1)
+            DebugSocket.stop
+          ensure
+            exit!(1)
+          end
         end
       end
     end
 
     it "catches errors in the debug socket thread" do
-      socket.write("asdf}(]")
-      socket.close_write
-      expect(socket.read).to eq("")
+      expect(write("asdf}(]")).to include("SyntaxError")
+      expect(write("2")).to eq("2\n")
 
-      another_socket = UNIXSocket.new(path)
-      another_socket.write("2")
-      another_socket.close_write
-      expect(another_socket.read).to eq("2\n")
-
-      expect(log_buffer.string).to include("debug-socket-error=#<SyntaxError: (eval):1: syntax error")
+      expect(log_buffer.string).to match(/debug-socket-error=#<SyntaxError:.*eval.*syntax error/)
       expect(log_buffer.string).to include('debug-socket-command="2"')
     end
 
-    context 'with proc' do
-      before do 
+    it "isolates the eval from the local scope" do
+      expect(write("server = 1")).to eq("1\n")
+      expect(write("server = 1")).to eq("1\n")
+    end
+
+    it "retries socket errors 10 times then dies" do
+      20.times { sleep(0.1) unless File.exist?(path) }
+
+      slept = false
+      allow(DebugSocket).to receive(:sleep).and_wrap_original do |original, delay|
+        next if slept
+
+        slept = true
+        original.call(delay)
+      end
+
+      socket = UNIXSocket.new(path)
+      socket.write("sleep(1)")
+      socket.close
+
+      20.times do
+        socket = UNIXSocket.new(path)
+        socket.close
+      end
+
+      almost_there(250) do
+        (1..20).each { |i| expect(log_buffer.string).to include("errors=#{i}") }
+        expect(log_buffer.string).to include("DebugSocket is broken now")
+      end
+    end
+
+    context "with proc" do
+      before do
         DebugSocket.stop
       end
 
@@ -95,10 +131,7 @@
         audit_proc = proc { |input| audit_calls << input }
 
         DebugSocket.start(path, &audit_proc)
-
-        socket.write("2 + 2")
-        socket.close_write
-        expect(socket.read).to eq("4\n")
+        expect(write("2 + 2")).to eq("4\n")
         expect(audit_calls).to eq(["2 + 2"])
       end
 
@@ -107,9 +140,7 @@
 
         DebugSocket.start(path, &audit_proc)
 
-        socket.write("3 + 3")
-        socket.close_write
-        expect(socket.read).to eq("6\n")
+        expect(write("3 + 3")).to eq("6\n")
         # No error should be raised to the client, and the command is processed
         expect(log_buffer.string).to include('debug-socket-error=callback unsuccessful: #<RuntimeError: audit error> for "3 + 3"')
       end
@@ -120,23 +151,23 @@
     it "returns a stacktrace for all threads" do
       time_pid = "\\d{4}-\\d\\d-\\d\\dT\\d\\d:\\d\\d:\\d\\dZ\\ pid=#{Process.pid}"
       running_thread = %r{#{time_pid}\ thread\.object_id=#{Thread.current.object_id}\ thread\.status=run\n
-        .*lib/debug_socket\.rb:\d+:in\ `backtrace'\n
-        .*lib/debug_socket\.rb:\d+:in\ `block\ in\ backtrace'\n
-        .*lib/debug_socket\.rb:\d+:in\ `map'\n
-        .*lib/debug_socket\.rb:\d+:in\ `backtrace'\n
-        .*spec/debug_socket_spec\.rb:\d+:in\ `block.*'}x
+        .*lib/debug_socket\.rb:\d+:in\ .*backtrace'\n
+        .*lib/debug_socket\.rb:\d+:in\ .*block\ in.*backtrace'\n
+        .*lib/debug_socket\.rb:\d+:in\ .*map'\n
+        .*lib/debug_socket\.rb:\d+:in\ .*backtrace'\n
+        .*spec/debug_socket_spec\.rb:\d+:in\ .*block.*'}x
       thread = Thread.new { sleep 1 }
       sleep 0.1
       sleeping_thread = %r{#{time_pid}\ thread\.object_id=#{thread.object_id}\ thread\.status=sleep\n
-        .*spec/debug_socket_spec\.rb:\d+:in\ `sleep'\n
-        .*spec/debug_socket_spec\.rb:\d+:in\ `block.*'}x
+        .*spec/debug_socket_spec\.rb:\d+:in\ .*sleep'\n
+        .*spec/debug_socket_spec\.rb:\d+:in\ .*block.*'}x
       bt = DebugSocket.backtrace
       expect(bt).to match(running_thread)
       expect(bt).to match(sleeping_thread)
     end
   end
 
-  describe "stress test", slow: true do
+  describe "stress test", :slow do
     it "works with lots of threads, even in jruby" do
       threads = Array.new(10) do
         Thread.new { 100.times { Thread.new { sleep(0.001) }.join } }

spec/spec_helper.rb

@@ -1,4 +1,18 @@
 # frozen_string_literal: true
 
-$LOAD_PATH.unshift File.expand_path("../lib", __dir__)
+require "bundler/setup"
 require "debug_socket"
+require "io/wait"
+require "pry-byebug"
+
+RSpec.configure do |_config|
+  def almost_there(retries = 100)
+    yield
+  rescue RSpec::Expectations::ExpectationNotMetError
+    raise if retries < 1
+
+    sleep 0.1
+    retries -= 1
+    retry
+  end
+end