Take #69, #70 and #71, each with a test

lib/net/protocol.rb

@@ -189,6 +189,7 @@ def close
     public
 
     def read(len, dest = ''.b, ignore_eof = false)
+      raise ArgumentError, "negative length #{len} given" if len < 0
       LOG "reading #{len} bytes..."
       read_bytes = 0
       begin
@@ -356,9 +357,9 @@ def writing
       @debug_output << '<- ' if @debug_output
       yield
       @debug_output << "\n" if @debug_output
-      bytes = @written_bytes
+      @written_bytes
+    ensure
       @written_bytes = nil
-      bytes
     end
 
     def write0(*strs)
@@ -384,6 +385,9 @@ def write0(*strs)
             # next string
           end
           # continue looping
+        when :wait_readable
+          (io = @io.to_io).wait_readable(@write_timeout) or raise Net::WriteTimeout.new(io)
+          # continue looping
         when :wait_writable
           (io = @io.to_io).wait_writable(@write_timeout) or raise Net::WriteTimeout.new(io)
           # continue looping
@@ -426,12 +430,15 @@ def initialize(*, **)
     def each_message_chunk
       LOG 'reading message...'
       LOG_off()
-      read_bytes = 0
-      while (line = readuntil("\r\n")) != ".\r\n"
-        read_bytes += line.size
-        yield line.delete_prefix('.')
+      begin
+        read_bytes = 0
+        while (line = readuntil("\r\n")) != ".\r\n"
+          read_bytes += line.size
+          yield line.delete_prefix('.')
+        end
+      ensure
+        LOG_on()
       end
-      LOG_on()
       LOG "read message (#{read_bytes} bytes)"
     end
 
@@ -457,29 +464,35 @@ def write_message_0(src)
     def write_message(src)
       LOG "writing message from #{src.class}"
       LOG_off()
-      len = writing {
-        using_each_crlf_line {
-          write_message_0 src
+      begin
+        len = writing {
+          using_each_crlf_line {
+            write_message_0 src
+          }
         }
-      }
-      LOG_on()
+      ensure
+        LOG_on()
+      end
       LOG "wrote #{len} bytes"
       len
     end
 
     def write_message_by_block(&block)
       LOG 'writing message from block'
       LOG_off()
-      len = writing {
-        using_each_crlf_line {
-          begin
-            block.call(WriteAdapter.new(self.method(:write_message_0)))
-          rescue LocalJumpError
-            # allow `break' from writer block
-          end
+      begin
+        len = writing {
+          using_each_crlf_line {
+            begin
+              block.call(WriteAdapter.new(self.method(:write_message_0)))
+            rescue LocalJumpError
+              # allow `break' from writer block
+            end
+          }
         }
-      }
-      LOG_on()
+      ensure
+        LOG_on()
+      end
       LOG "wrote #{len} bytes"
       len
     end
@@ -499,6 +512,8 @@ def using_each_crlf_line
         write0 "\r\n"
       end
       write0 ".\r\n"
+      nil
+    ensure
       @wbuf = nil
     end
 

test/net/protocol/test_protocol.rb

@@ -310,4 +310,120 @@ def test_readuntil_ignore_eof_returns_what_is_left # https://github.com/ruby/net
     assert_equal "ab\r", io.readuntil("\r")
     assert_equal "\n\r\nc", io.readuntil("\r\n\r\n", true)
   end
+
+  # The length reaches rbuf_consume, which walks @rbuf_offset backwards by
+  # it, so a negative one has to be rejected before the buffer moves.
+  def test_read_rejects_a_negative_length # https://github.com/ruby/net-protocol/pull/69
+    io = Net::BufferedIO.new(StringIO.new("abcdef".dup))
+    assert_equal "a", io.read(1)
+    e = assert_raise(ArgumentError) { io.read(-1) }
+    assert_equal "negative length -1 given", e.message
+    assert_equal "bcdef", io.read_all
+  end
+
+  # OpenSSL::Buffering#write_nonblock documents :wait_readable, which a
+  # renegotiation produces. Takes the write only once the caller has
+  # waited, and caps the attempts so a regression fails instead of
+  # spinning until CI gives up.
+  class WaitReadableWriteIO
+    MAX_WRITES = 3
+
+    attr_reader :string, :waits
+
+    def initialize(becomes_readable: true)
+      @becomes_readable = becomes_readable
+      @string = "".b
+      @writes = 0
+      @waits = 0
+    end
+
+    def to_io; self; end
+
+    def wait_readable(_timeout)
+      @waits += 1
+      @becomes_readable
+    end
+
+    def write_nonblock(str, exception: true)
+      @writes += 1
+      raise "write0 ignored :wait_readable: #{@writes} attempts" if @writes > MAX_WRITES
+      return :wait_readable if @waits.zero?
+      @string << str
+      str.bytesize
+    end
+  end
+
+  def test_write0_waits_for_readability # https://github.com/ruby/net-protocol/pull/70
+    mockio = WaitReadableWriteIO.new
+    io = Net::BufferedIO.new(mockio)
+    io.write_timeout = 0.1
+    assert_equal 5, io.write("hello")
+    assert_equal "hello", mockio.string
+    assert_equal 1, mockio.waits
+  end
+
+  def test_write0_times_out_waiting_for_readability # https://github.com/ruby/net-protocol/pull/70
+    mockio = WaitReadableWriteIO.new(becomes_readable: false)
+    io = Net::BufferedIO.new(mockio)
+    io.write_timeout = 0.1
+    assert_raise(Net::WriteTimeout) { io.write("hello") }
+    assert_equal 1, mockio.waits
+  end
+
+  def test_write_message_by_block # https://github.com/ruby/net-protocol/pull/71
+    sio = StringIO.new("".dup)
+    imio = Net::InternetMessageIO.new(sio)
+    assert_equal 10, imio.write_message_by_block { |dest| dest.write("hello\r\n") }
+    assert_equal "hello\r\n.\r\n", sio.string
+  end
+
+  # `break' leaves through write_message_by_block itself, so the message
+  # stays unterminated and the method answers nil.
+  def test_write_message_by_block_allows_break # https://github.com/ruby/net-protocol/pull/71
+    sio = StringIO.new("".dup)
+    imio = Net::InternetMessageIO.new(sio)
+    assert_nil imio.write_message_by_block { |dest| dest.write("a\r\n"); break }
+    assert_equal "a\r\n", sio.string
+  end
+
+  def test_write_message_by_block_restores_logging_when_the_block_raises # https://github.com/ruby/net-protocol/pull/71
+    sio = StringIO.new("".dup)
+    imio = Net::InternetMessageIO.new(sio)
+    debug = "".dup
+    imio.debug_output = debug
+
+    assert_raise(RuntimeError) do
+      imio.write_message_by_block { |dest| dest.write("partial"); raise "boom" }
+    end
+
+    assert_same debug, imio.debug_output
+    # Nothing outside reaches the half-written line or the byte count, so
+    # they have to be read back from the inside.
+    assert_nil imio.instance_variable_get(:@wbuf)
+    assert_nil imio.instance_variable_get(:@written_bytes)
+  end
+
+  def test_write_message_restores_logging_when_the_source_raises # https://github.com/ruby/net-protocol/pull/71
+    sio = StringIO.new("".dup)
+    imio = Net::InternetMessageIO.new(sio)
+    debug = "".dup
+    imio.debug_output = debug
+
+    src = Object.new
+    def src.each; yield "partial"; raise "boom"; end
+
+    assert_raise(RuntimeError) { imio.write_message(src) }
+
+    assert_same debug, imio.debug_output
+  end
+
+  def test_each_message_chunk_restores_logging_when_the_block_raises # https://github.com/ruby/net-protocol/pull/71
+    imio = Net::InternetMessageIO.new(StringIO.new("line\r\n.\r\n".dup))
+    debug = "".dup
+    imio.debug_output = debug
+
+    assert_raise(RuntimeError) { imio.each_message_chunk { raise "boom" } }
+
+    assert_same debug, imio.debug_output
+  end
 end