From 045715a3ca93427406cb92cd2b64e6552302aa4b Mon Sep 17 00:00:00 2001 From: Doug Mayer Date: Tue, 14 Feb 2023 15:17:51 -0600 Subject: [PATCH] Wrap likely ABQ network errors to more easily identify as coming from ABQ connections. (#104) * Wrap likely ABQ network errors to more easily identify as coming from ABQ connections. * Fix socket opening for Ruby <3.0. * Remove resolv_timeout for Ruby 2.6 support. * Bump version 1.1.1 * Update test gemfiles. --- .rubocop.yml | 3 ++ Gemfile.lock | 10 +++--- gemfiles/rspec-3.10.gemfile.lock | 2 +- gemfiles/rspec-3.11.gemfile.lock | 2 +- gemfiles/rspec-3.12.gemfile.lock | 2 +- gemfiles/rspec-3.5.gemfile.lock | 2 +- gemfiles/rspec-3.6.gemfile.lock | 2 +- gemfiles/rspec-3.7.gemfile.lock | 2 +- gemfiles/rspec-3.8.gemfile.lock | 2 +- gemfiles/rspec-3.9.gemfile.lock | 2 +- lib/rspec/abq.rb | 55 ++++++++++++++++++++------------ lib/rspec/abq/version.rb | 2 +- spec/NUM_TESTS | 2 +- spec/rspec/abq_spec.rb | 38 ++++++++++++++++++++-- 14 files changed, 88 insertions(+), 38 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index f3d6b03f..3fc8f3ff 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -24,3 +24,6 @@ RSpec/ExampleLength: RSpec/MessageSpies: EnforcedStyle: receive + +RSpec/MultipleMemoizedHelpers: + Max: 10 diff --git a/Gemfile.lock b/Gemfile.lock index 74563984..2b797c1f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: . specs: - rspec-abq (1.1.0) + rspec-abq (1.1.1) rspec-core (>= 3.5.0, < 3.13.0) GEM @@ -95,8 +95,8 @@ GEM rubocop-ast (>= 1.24.1, < 2.0) ruby-progressbar (~> 1.7) unicode-display_width (>= 2.4.0, < 3.0) - rubocop-ast (1.24.1) - parser (>= 3.1.1.0) + rubocop-ast (1.26.0) + parser (>= 3.2.1.0) rubocop-capybara (2.17.0) rubocop (~> 1.41) rubocop-performance (1.15.2) @@ -123,8 +123,8 @@ GEM simplecov_json_formatter (~> 0.1) simplecov-html (0.12.3) simplecov_json_formatter (0.1.4) - sorbet-runtime (0.5.10655) - standard (1.24.0) + sorbet-runtime (0.5.10662) + standard (1.24.3) language_server-protocol (~> 3.17.0.2) rubocop (= 1.44.1) rubocop-performance (= 1.15.2) diff --git a/gemfiles/rspec-3.10.gemfile.lock b/gemfiles/rspec-3.10.gemfile.lock index ab5db880..124e2475 100644 --- a/gemfiles/rspec-3.10.gemfile.lock +++ b/gemfiles/rspec-3.10.gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: .. specs: - rspec-abq (1.1.0) + rspec-abq (1.1.1) rspec-core (>= 3.5.0, < 3.13.0) GEM diff --git a/gemfiles/rspec-3.11.gemfile.lock b/gemfiles/rspec-3.11.gemfile.lock index 067eb53d..5985760a 100644 --- a/gemfiles/rspec-3.11.gemfile.lock +++ b/gemfiles/rspec-3.11.gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: .. specs: - rspec-abq (1.1.0) + rspec-abq (1.1.1) rspec-core (>= 3.5.0, < 3.13.0) GEM diff --git a/gemfiles/rspec-3.12.gemfile.lock b/gemfiles/rspec-3.12.gemfile.lock index f74868b1..6f5bb3a2 100644 --- a/gemfiles/rspec-3.12.gemfile.lock +++ b/gemfiles/rspec-3.12.gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: .. specs: - rspec-abq (1.1.0) + rspec-abq (1.1.1) rspec-core (>= 3.5.0, < 3.13.0) GEM diff --git a/gemfiles/rspec-3.5.gemfile.lock b/gemfiles/rspec-3.5.gemfile.lock index edc67a2b..b776047a 100644 --- a/gemfiles/rspec-3.5.gemfile.lock +++ b/gemfiles/rspec-3.5.gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: .. specs: - rspec-abq (1.1.0) + rspec-abq (1.1.1) rspec-core (>= 3.5.0, < 3.13.0) GEM diff --git a/gemfiles/rspec-3.6.gemfile.lock b/gemfiles/rspec-3.6.gemfile.lock index 70298697..6f47c2c4 100644 --- a/gemfiles/rspec-3.6.gemfile.lock +++ b/gemfiles/rspec-3.6.gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: .. specs: - rspec-abq (1.1.0) + rspec-abq (1.1.1) rspec-core (>= 3.5.0, < 3.13.0) GEM diff --git a/gemfiles/rspec-3.7.gemfile.lock b/gemfiles/rspec-3.7.gemfile.lock index efd3dcf2..9532f6db 100644 --- a/gemfiles/rspec-3.7.gemfile.lock +++ b/gemfiles/rspec-3.7.gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: .. specs: - rspec-abq (1.1.0) + rspec-abq (1.1.1) rspec-core (>= 3.5.0, < 3.13.0) GEM diff --git a/gemfiles/rspec-3.8.gemfile.lock b/gemfiles/rspec-3.8.gemfile.lock index b0bfb2ce..49838d12 100644 --- a/gemfiles/rspec-3.8.gemfile.lock +++ b/gemfiles/rspec-3.8.gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: .. specs: - rspec-abq (1.1.0) + rspec-abq (1.1.1) rspec-core (>= 3.5.0, < 3.13.0) GEM diff --git a/gemfiles/rspec-3.9.gemfile.lock b/gemfiles/rspec-3.9.gemfile.lock index fe46b6a8..b195410b 100644 --- a/gemfiles/rspec-3.9.gemfile.lock +++ b/gemfiles/rspec-3.9.gemfile.lock @@ -9,7 +9,7 @@ GIT PATH remote: .. specs: - rspec-abq (1.1.0) + rspec-abq (1.1.1) rspec-core (>= 3.5.0, < 3.13.0) GEM diff --git a/lib/rspec/abq.rb b/lib/rspec/abq.rb index a40d1932..1e48d0db 100644 --- a/lib/rspec/abq.rb +++ b/lib/rspec/abq.rb @@ -148,8 +148,20 @@ def self.setup_extensions_if_enabled! Extensions.setup! end + # Base ABQ error. + Error = Class.new(StandardError) + # raised when check_configuration fails - UnsupportedConfigurationError = Class.new(StandardError) + UnsupportedConfigurationError = Class.new(Error) + + # Failed to connect and initialize handshake. + ConnectionFailed = Class.new(Error) + + # Communication between abq sockets follows the following protocol: + # - The first 4 bytes an unsigned 32-bit integer (big-endian) representing + # the size of the rest of the message. + # - The rest of the message is a JSON-encoded payload. + ConnectionBroken = Class.new(Error) # raises if RSpec is configured in a way that's incompatible with rspec-abq def self.check_configuration!(config) @@ -214,12 +226,16 @@ def self.fast_exit? # Creates the socket to communicate with the worker and sends the worker the protocol # @!visibility private - def self.socket - @socket ||= TCPSocket.new(*ENV[ABQ_SOCKET].split(":")).tap do |socket| - # Messages sent to/received from the ABQ worker should be done so ASAP. - # Since we're on a local network, we don't care about packet reduction here. - socket.setsockopt(Socket::IPPROTO_TCP, Socket::TCP_NODELAY, 1) - protocol_write(NATIVE_RUNNER_SPAWNED_MESSAGE, socket) + def self.socket(connect_timeout: 5) + @socket ||= begin + Socket.tcp(*ENV[ABQ_SOCKET].split(":"), connect_timeout: connect_timeout).tap do |socket| + # Messages sent to/received from the ABQ worker should be done so ASAP. + # Since we're on a local network, we don't care about packet reduction here. + socket.setsockopt(Socket::IPPROTO_TCP, Socket::TCP_NODELAY, 1) + protocol_write(NATIVE_RUNNER_SPAWNED_MESSAGE, socket) + end + rescue + raise ConnectionFailed, "Unable to connect to ABQ socket #{ENV[ABQ_SOCKET]}" end end @@ -245,25 +261,18 @@ def self.fetch_next_example(message = protocol_read) end end - # Communication between abq sockets follows the following protocol: - # - The first 4 bytes an unsigned 32-bit integer (big-endian) representing - # the size of the rest of the message. - # - The rest of the message is a JSON-encoded payload. - class AbqConnBroken < StandardError - end - # Writes a message to an Abq socket using the 4-byte header protocol. # # @param socket [TCPSocket] # @param msg def self.protocol_write(msg, socket = Abq.socket) json_msg = JSON.dump msg - begin - socket.write [json_msg.bytesize].pack("N") - socket.write json_msg - rescue - raise AbqConnBroken - end + socket.write [json_msg.bytesize].pack("N") + socket.write json_msg + rescue SystemCallError, IOError + raise ConnectionBroken + rescue + raise Error end # Writes a message to an Abq socket using the 4-byte header protocol. @@ -273,10 +282,16 @@ def self.protocol_write(msg, socket = Abq.socket) def self.protocol_read(socket = Abq.socket) len_bytes = socket.read 4 return :abq_done if len_bytes.nil? + len = len_bytes.unpack1("N") json_msg = socket.read len return :abq_done if json_msg.nil? + JSON.parse json_msg + rescue SystemCallError, IOError + raise ConnectionBroken + rescue + raise Error end end end diff --git a/lib/rspec/abq/version.rb b/lib/rspec/abq/version.rb index ec7adfd6..5e36b525 100644 --- a/lib/rspec/abq/version.rb +++ b/lib/rspec/abq/version.rb @@ -1,6 +1,6 @@ module RSpec module Abq # current version! - VERSION = "1.1.0" + VERSION = "1.1.1" end end diff --git a/spec/NUM_TESTS b/spec/NUM_TESTS index 920a1396..9e5feb52 100644 --- a/spec/NUM_TESTS +++ b/spec/NUM_TESTS @@ -1 +1 @@ -43 +46 diff --git a/spec/rspec/abq_spec.rb b/spec/rspec/abq_spec.rb index 0951f847..e0445de1 100644 --- a/spec/rspec/abq_spec.rb +++ b/spec/rspec/abq_spec.rb @@ -78,13 +78,14 @@ def stringify_keys(hash) end describe "socket communication", unless: RSpec::Abq.disable_tests_when_run_by_abq? do - host = "127.0.0.1" + let(:host) { "127.0.0.1" } + let(:port) { server.addr[1] } let(:server) { TCPServer.new(host, 0) } - let(:client_sock) { TCPSocket.new(host, server.addr[1]) } + let(:client_sock) { TCPSocket.new(host, port) } let(:server_sock) { server.accept } around do |example| - EnvHelper.with_env("ABQ_SOCKET" => "#{host}:#{server.addr[1]}") do + EnvHelper.with_env("ABQ_SOCKET" => "#{host}:#{port}") do example.call end RSpec::Abq.instance_eval { @socket = nil } @@ -99,6 +100,15 @@ def stringify_keys(hash) expect(server_sock.read(4).unpack1("N")).to eq(payload_length) expect(server_sock.read(payload_length)).to eq(symbol_payload.to_json) end + + it "wraps errors with ConnectionBroken" do + socket = client_sock # touch before closing server + server.close + + expect do + RSpec::Abq.protocol_write(symbol_payload, socket) + end.to raise_error(RSpec::Abq::ConnectionBroken) + end end describe ".protocol_read" do @@ -109,6 +119,18 @@ def stringify_keys(hash) expect(RSpec::Abq.protocol_read(server_sock)).to eq(JSON.parse(msg_payload)) end + + it "wraps errors with ConnectionBroken" do + msg_payload = '{"a":1,"b":2}' + client_sock.write([msg_payload.length].pack("N")) + client_sock.write(msg_payload) + + server_sock.close + + expect do + RSpec::Abq.protocol_read(server_sock) + end.to raise_error(RSpec::Abq::ConnectionBroken) + end end describe ".socket" do @@ -119,6 +141,16 @@ def stringify_keys(hash) RSpec::Abq.protocol_write({"init_meta" => {"seed" => 4, "ordering_class" => "RSpec::Core::Ordering::Identity"}}, server_sock) expect(RSpec::Abq.protocol_read(server_sock)).to(eq(stringify_keys(RSpec::Abq::NATIVE_RUNNER_SPAWNED_MESSAGE))) end + + it "fails with ConnectionFailed when connection times out" do + # Force this error to avoid flakiness. + allow(Socket).to receive(:tcp).with(host, port.to_s, connect_timeout: 0.001) + .and_raise(Errno::ETIMEDOUT, "forced error") + + expect do + RSpec::Abq.socket(connect_timeout: 0.001) + end.to raise_error(RSpec::Abq::ConnectionFailed) + end end end