buffer wraps string (#65)

* buffer wraps string

* remove buffer creation in specs

* Set rubocop version

Gemfile

@@ -10,7 +10,7 @@ group :test do
   gem 'pry-byebug', platform: :mri
   gem 'rspec', '~> 3.4.0'
   gem 'rspec-autotest'
-  gem 'rubocop', '~> 0.43'
+  gem 'rubocop', '0.43.0'
 end
 
 gemspec

lib/http/2/buffer.rb

@@ -1,35 +1,61 @@
+require 'forwardable'
+
 module HTTP2
-  # Simple binary buffer backed by string.
-  #
-  # TODO: Refactor, it would be better if Buffer were not a String subclass,
-  # but rather wrap a string and only expose the mutating API needed so that
-  # the possible surface for things to go wrong stays controllable.
-  # - https://github.com/igrigorik/http-2/pull/46
+  # Binary buffer wraps String.
   #
-  class Buffer < String
+  class Buffer
+    extend Forwardable
+
+    def_delegators :@buffer, :ord, :encoding, :setbyte, :unpack,
+                   :size, :each_byte,  :to_str, :length, :inspect,
+                   :[], :[]=, :empty?, :bytesize, :include?
+
     UINT32 = 'N'.freeze
     private_constant :UINT32
 
     # Forces binary encoding on the string
-    def initialize(*)
-      super.force_encoding(Encoding::BINARY)
+    def initialize(str = '')
+      str = str.dup if str.frozen?
+      @buffer = str.force_encoding(Encoding::BINARY)
     end
 
     # Emulate StringIO#read: slice first n bytes from the buffer.
     #
     # @param n [Integer] number of bytes to slice from the buffer
     def read(n)
-      Buffer.new(slice!(0, n))
+      Buffer.new(@buffer.slice!(0, n))
     end
 
-    # Alias getbyte to readbyte
-    alias readbyte getbyte
-
     # Emulate StringIO#getbyte: slice first byte from buffer.
     def getbyte
       read(1).ord
     end
 
+    def slice!(*args)
+      Buffer.new(@buffer.slice!(*args))
+    end
+
+    def slice(*args)
+      Buffer.new(@buffer.slice(*args))
+    end
+
+    def force_encoding(*args)
+      @buffer = @buffer.force_encoding(*args)
+    end
+
+    def ==(other)
+      @buffer == other
+    end
+
+    def +(other)
+      @buffer += other
+    end
+
+    # Emulate String#getbyte: return nth byte from buffer.
+    def readbyte(n)
+      @buffer[n].ord
+    end
+
     # Slice unsigned 32-bit integer from buffer.
     # @return [Integer]
     def read_uint32
@@ -41,7 +67,9 @@ def read_uint32
     [:<<, :prepend].each do |mutating_method|
       define_method(mutating_method) do |string|
         string = string.dup if string.frozen?
-        super(string.force_encoding(Encoding::BINARY))
+        @buffer.send mutating_method, string.force_encoding(Encoding::BINARY)
+
+        self
       end
     end
   end

lib/http/2/connection.rb

@@ -574,7 +574,7 @@ def connection_settings(frame)
     #
     # @param frame [Hash]
     def decode_headers(frame)
-      if frame[:payload].is_a? String
+      if frame[:payload].is_a? Buffer
         frame[:payload] = @decompressor.decode(frame[:payload])
       end
 
@@ -588,7 +588,7 @@ def decode_headers(frame)
     # @return [Array of Frame]
     def encode_headers(frame)
       payload = frame[:payload]
-      payload = @compressor.encode(payload) unless payload.is_a? String
+      payload = @compressor.encode(payload) unless payload.is_a? Buffer
 
       frames = []
 

spec/client_spec.rb

@@ -56,10 +56,10 @@
     it 'should raise error on PUSH_PROMISE against non-idle stream' do
       expect do
         s = @client.new_stream
-        s.send HEADERS
+        s.send HEADERS.dup
 
-        @client << set_stream_id(f.generate(PUSH_PROMISE), s.id)
-        @client << set_stream_id(f.generate(PUSH_PROMISE), s.id)
+        @client << set_stream_id(f.generate(PUSH_PROMISE.dup), s.id)
+        @client << set_stream_id(f.generate(PUSH_PROMISE.dup), s.id)
       end.to raise_error(ProtocolError)
     end
 

spec/connection_spec.rb

@@ -149,7 +149,6 @@
       ]
       headers = []
       @conn.on(:frame) do |bytes|
-        # bytes[3]: frame's type field
         headers << f.parse(bytes) if [1, 5, 9].include?(bytes[3].ord)
       end