Merge branch 'topic/client-redesign-api'

* topic/client-redesign-api:
  readme: fix the flow chart showing how it connects to the server
  readme/example: replace obsolete Client#[http-method]! examples
  client: add Response#join method
  client: call the block passed to Client#request in Response#set_headers
  client: make Response's internal methods private
  client: Client#resume always waits all requests
  client: remove synchronous HTTP method methods
  client: OpenSSL::SSL::SSLContext always responds to hostname=
  client: replace 'scheme' option with 'https' option
  client: remove http2 (enable or disable HTTP/2) option

9 files changed, 157 insertions, 168 deletions

diff --git a/README.md b/README.md
index b8bba11..de36a33 100644
--- a/README.md
+++ b/README.md
@@ -51,15 +51,11 @@ If the server does't support HTTP/2, `Plum::Client` tries to use HTTP/1.x instea
 
 ```
    +-----------------+
-   |:http2 option    | false
-   |(default: true)  |-------> HTTP/1.x
+   |:https option    | false
+   |(default: true)  |-------> Try Upgrade from HTTP/1.1
    +-----------------+
-            v true
-   +-----------------+
-   |:scheme option   | "http"
-   |(default:"https")|-------> Try Upgrade from HTTP/1.1
-   +-----------------+
-            v "https"
+            | true
+            v
    +-----------------+
    | ALPN            | failed
    | negotiation     |-------> HTTP/1.x
@@ -71,10 +67,10 @@ If the server does't support HTTP/2, `Plum::Client` tries to use HTTP/1.x instea
 
 ##### Sequential request
 ```ruby
-client = Plum::Client.start("http2.rhe.jp", 443, user_agent: "nyaan")
-res1 = client.get!("/", headers: { "accept" => "*/*" })
+client = Plum::Client.start("http2.rhe.jp", user_agent: "nyaan")
+res1 = client.get("/", headers: { "accept" => "*/*" }).join
 puts res1.body # => "..."
-res2 = client.post!("/post", "data")
+res2 = client.post("/post", "data").join
 puts res2.body # => "..."
 
 client.close
diff --git a/examples/client/synchronous.rb b/examples/client/synchronous.rb
new file mode 100644
index 0000000..6701e1e
--- /dev/null
+++ b/examples/client/synchronous.rb
@@ -0,0 +1,24 @@
+# frozen-string-literal: true
+# client/synchronous.rb: download 3 files in sequence
+$LOAD_PATH.unshift File.expand_path("../../../lib", __FILE__)
+require "plum"
+require "zlib"
+
+client = Plum::Client.start("http2.golang.org", 443)
+
+reqinfo = client.get("/reqinfo").join
+puts "/reqinfo: #{reqinfo.status}"
+
+test = "test"
+crc32 = client.put("/crc32", test).join
+puts "/crc32{#{test}}: #{crc32.body}"
+puts "Zlib.crc32: #{Zlib.crc32(test).to_s(16)}"
+
+client.get("/clockstream")
+  .on_headers { |res|
+  puts "status: #{res.status}, headers: #{res.headers}"
+  }.on_chunk { |chunk|
+    puts chunk
+  }.on_finish {
+    puts "finish!"
+  }.join
diff --git a/examples/client/twitter.rb b/examples/client/twitter.rb
index d31c15a..a67fad3 100644
--- a/examples/client/twitter.rb
+++ b/examples/client/twitter.rb
@@ -41,10 +41,12 @@ Plum::Client.start("userstream.twitter.com", 443) { |streaming|
         if /にゃーん/ =~ json["text"]
           args = { "status" => "@#{json["user"]["screen_name"]} にゃーん",
                    "in_reply_to_status_id" => json["id"].to_s }
-          rest.post!("/1.1/statuses/update.json",
-                     args.map { |k, v| "#{k}=#{CGI.escape(v)}" }.join("&"),
-                     headers: { "authorization" => SimpleOAuth::Header.new(:post, "https://api.twitter.com/1.1/statuses/update.json", args, credentials).to_s,
-                                "content-type" => "application/x-www-form-urlencoded" })
+          rest.post(
+            "/1.1/statuses/update.json",
+            args.map { |k, v| "#{k}=#{CGI.escape(v)}" }.join("&"),
+            headers: { "authorization" => SimpleOAuth::Header.new(:post, "https://api.twitter.com/1.1/statuses/update.json", args, credentials).to_s,
+                       "content-type" => "application/x-www-form-urlencoded" }
+          ).join
         end
       end
     }
diff --git a/lib/plum/client.rb b/lib/plum/client.rb
index 959b445..d0e6e56 100644
--- a/lib/plum/client.rb
+++ b/lib/plum/client.rb
@@ -3,8 +3,7 @@
 module Plum
   class Client
     DEFAULT_CONFIG = {
-      http2: true,
-      scheme: "https",
+      https: true,
       hostname: nil,
       verify_mode: OpenSSL::SSL::VERIFY_PEER,
       ssl_context: nil,
@@ -33,9 +32,9 @@ module Plum
       else
         @socket = nil
         @host = host
-        @port = port || (config[:scheme] == "https" ? 443 : 80)
+        @port = port || (config[:https] ? 443 : 80)
       end
-      @config = DEFAULT_CONFIG.merge(hostname: host).merge(config)
+      @config = DEFAULT_CONFIG.merge(hostname: host).merge!(config)
       @started = false
     end
 
@@ -56,16 +55,9 @@ module Plum
       self
     end
 
-    # Resume communication with the server, until the specified (or all running) requests are complete.
-    # @param response [Response] if specified, waits only for the response
-    # @return [Response] if parameter response is specified
-    def resume(response = nil)
-      if response
-        @session.succ until response.failed? || response.finished?
-        response
-      else
-        @session.succ until @session.empty?
-      end
+    # Resume communication with the server until all running requests are complete.
+    def resume
+      @session.succ until @session.empty?
     end
 
     # Closes the connection immediately.
@@ -85,14 +77,6 @@ module Plum
       @session.request(headers, body, @config.merge(options), &block)
     end
 
-    # @!method get!
-    # @!method head!
-    # @!method delete!
-    # @param path [String] the absolute path to request (translated into :path header)
-    # @param options [Hash<Symbol, Object>] the request options
-    # @param block [Proc] if specified, calls the block when finished
-    # Shorthand method for `Client#resume(Client#request(*args))`
-
     # @!method get
     # @!method head
     # @!method delete
@@ -101,20 +85,10 @@ module Plum
     # @param block [Proc] if specified, calls the block when finished
     # Shorthand method for `#request`
     %w(GET HEAD DELETE).each { |method|
-      define_method(:"#{method.downcase}!") do |path, options = {}, &block|
-        resume _request_helper(method, path, nil, options, &block)
-      end
       define_method(:"#{method.downcase}") do |path, options = {}, &block|
         _request_helper(method, path, nil, options, &block)
       end
     }
-    # @!method post!
-    # @!method put!
-    # @param path [String] the absolute path to request (translated into :path header)
-    # @param body [String] the request body
-    # @param options [Hash<Symbol, Object>] the request options
-    # @param block [Proc] if specified, calls the block when finished
-    # Shorthand method for `Client#resume(Client#request(*args))`
 
     # @!method post
     # @!method put
@@ -124,9 +98,6 @@ module Plum
     # @param block [Proc] if specified, calls the block when finished
     # Shorthand method for `#request`
     %w(POST PUT).each { |method|
-      define_method(:"#{method.downcase}!") do |path, body, options = {}, &block|
-        resume _request_helper(method, path, body, options, &block)
-      end
       define_method(:"#{method.downcase}") do |path, body, options = {}, &block|
         _request_helper(method, path, body, options, &block)
       end
@@ -137,13 +108,15 @@ module Plum
     def _connect
       @socket = TCPSocket.open(@host, @port)
 
-      if @config[:scheme] == "https"
+      if @config[:https]
         ctx = @config[:ssl_context] || new_ssl_ctx
         @socket = OpenSSL::SSL::SSLSocket.new(@socket, ctx)
-        @socket.hostname = @config[:hostname] if @socket.respond_to?(:hostname=)
+        @socket.hostname = @config[:hostname]
         @socket.sync_close = true
         @socket.connect
-        @socket.post_connection_check(@config[:hostname]) if ctx.verify_mode != OpenSSL::SSL::VERIFY_NONE
+        if ctx.verify_mode != OpenSSL::SSL::VERIFY_NONE
+          @socket.post_connection_check(@config[:hostname])
+        end
 
         @socket.alpn_protocol == "h2"
       end
@@ -151,18 +124,12 @@ module Plum
 
     def _start
       @started = true
-
-      klass = @config[:http2] ? ClientSession : LegacyClientSession
       nego = @socket || _connect
 
-      if @config[:http2]
-        if @config[:scheme] == "https"
-          klass = nego ? ClientSession : LegacyClientSession
-        else
-          klass = UpgradeClientSession
-        end
+      if @config[:https]
+        klass = nego ? ClientSession : LegacyClientSession
       else
-        klass = LegacyClientSession
+        klass = UpgradeClientSession
       end
 
       @session = klass.new(@socket, @config)
@@ -175,10 +142,8 @@ module Plum
       cert_store = OpenSSL::X509::Store.new
       cert_store.set_default_paths
       ctx.cert_store = cert_store
-      if @config[:http2]
-        ctx.ciphers = "ALL:!" + SSLSocketServerConnection::CIPHER_BLACKLIST.join(":!")
-        ctx.alpn_protocols = ["h2", "http/1.1"]
-      end
+      ctx.ciphers = "ALL:!" + SSLSocketServerConnection::CIPHER_BLACKLIST.join(":!")
+      ctx.alpn_protocols = ["h2", "http/1.1"]
       ctx
     end
 
diff --git a/lib/plum/client/client_session.rb b/lib/plum/client/client_session.rb
index 10276d4..81d2341 100644
--- a/lib/plum/client/client_session.rb
+++ b/lib/plum/client/client_session.rb
@@ -31,7 +31,7 @@ module Plum
 
     def close
       @closed = true
-      @responses.each(&:_fail)
+      @responses.each { |res| res.send(:fail) }
       @responses.clear
       @plum.close
     end
@@ -40,34 +40,34 @@ module Plum
       headers = { ":method" => nil,
                   ":path" => nil,
                   ":authority" => @config[:hostname],
-                  ":scheme" => @config[:scheme]
+                  ":scheme" => @config[:https] ? "https" : "http",
       }.merge(headers)
 
-      response = Response.new(**options)
+      response = Response.new(self, **options, &headers_cb)
       @responses << response
       stream = @plum.open_stream
       stream.send_headers(headers, end_stream: !body)
       stream.send_data(body, end_stream: true) if body
 
       stream.on(:headers) { |resp_headers_raw|
-        response._headers(resp_headers_raw)
-        headers_cb.call(response) if headers_cb
+        response.send(:set_headers, resp_headers_raw.to_h)
       }
       stream.on(:data) { |chunk|
-        response._chunk(chunk)
+        response.send(:add_chunk, chunk)
         check_window(stream)
       }
       stream.on(:end_stream) {
-        response._finish
+        response.send(:finish)
         @responses.delete(response)
       }
       stream.on(:stream_error) { |ex|
-        response._fail
+        response.send(:fail, ex)
         raise ex
       }
       stream.on(:local_stream_error) { |type|
-        response.fail
-        raise LocalStreamError.new(type)
+        ex = LocalStreamError.new(type)
+        response.send(:fail, ex)
+        raise ex
       }
       response
     end
diff --git a/lib/plum/client/legacy_client_session.rb b/lib/plum/client/legacy_client_session.rb
index e9ff20b..a12f582 100644
--- a/lib/plum/client/legacy_client_session.rb
+++ b/lib/plum/client/legacy_client_session.rb
@@ -12,7 +12,6 @@ module Plum
       @parser = setup_parser
       @requests = []
       @response = nil
-      @headers_callback = nil
     end
 
     def succ
@@ -27,7 +26,7 @@ module Plum
 
     def close
       @closed = true
-      @response._fail if @response
+      @response.send(:fail) if @response
     end
 
     def request(headers, body, options, &headers_cb)
@@ -41,8 +40,8 @@ module Plum
         end
       end
 
-      response = Response.new(**options)
-      @requests << [response, headers, body, chunked, headers_cb]
+      response = Response.new(self, **options, &headers_cb)
+      @requests << [response, headers, body, chunked]
       consume_queue
       response
     end
@@ -56,9 +55,8 @@ module Plum
     def consume_queue
       return if @response || @requests.empty?
 
-      response, headers, body, chunked, cb = @requests.shift
+      response, headers, body, chunked = @requests.shift
       @response = response
-      @headers_callback = cb
 
       @socket << construct_request(headers)
 
@@ -96,19 +94,18 @@ module Plum
     def setup_parser
       parser = HTTP::Parser.new
       parser.on_headers_complete = proc {
+        # FIXME: duplicate header name?
         resp_headers = parser.headers.map { |key, value| [key.downcase, value] }.to_h
-        @response._headers({ ":status" => parser.status_code.to_s }.merge(resp_headers))
-        @headers_callback.call(@response) if @headers_callback
+        @response.send(:set_headers, { ":status" => parser.status_code.to_s }.merge(resp_headers))
       }
 
       parser.on_body = proc { |chunk|
-        @response._chunk(chunk)
+        @response.send(:add_chunk, chunk)
       }
 
       parser.on_message_complete = proc { |env|
-        @response._finish
+        @response.send(:finish)
         @response = nil
-        @headers_callback = nil
         close unless parser.keep_alive?
         consume_queue
       }
diff --git a/lib/plum/client/response.rb b/lib/plum/client/response.rb
index ce263f7..e937fa6 100644
--- a/lib/plum/client/response.rb
+++ b/lib/plum/client/response.rb
@@ -7,19 +7,21 @@ module Plum
     attr_reader :headers
 
     # @api private
-    def initialize(auto_decode: true, **options)
-      @body = Queue.new
+    def initialize(session, auto_decode: true, **options, &on_headers)
+      @session = session
+      @headers = nil
       @finished = false
       @failed = false
       @body = []
       @auto_decode = auto_decode
+      @on_headers = on_headers
       @on_chunk = @on_finish = nil
     end
 
     # Returns the HTTP status code.
     # @return [String] the HTTP status code
     def status
-      @headers && @headers[":status"]
+      @headers&.fetch(":status")
     end
 
     # Returns the header value that correspond to the header name.
@@ -41,7 +43,16 @@ module Plum
       @failed
     end
 
-    # Set callback tha called when received a chunk of response body.
+    # Set callback that will be called when the response headers arrive
+    # @yield [self]
+    def on_headers(&block)
+      raise ArgumentError, "block must be given" unless block_given?
+      @on_headers = block
+      yield self if @headers
+      self
+    end
+
+    # Set callback that will be called when received a chunk of response body.
     # @yield [chunk] A chunk of the response body.
     def on_chunk(&block)
       raise "Body already read" if @on_chunk
@@ -51,6 +62,7 @@ module Plum
         @body.each(&block)
         @body.clear
       end
+      self
     end
 
     # Set callback that will be called when the response finished.
@@ -61,6 +73,7 @@ module Plum
       else
         @on_finish = block
       end
+      self
     end
 
     # Returns the complete response body. Use #each_body instead if the body can be very large.
@@ -71,15 +84,20 @@ module Plum
       @body.join
     end
 
-    # @api private
-    def _headers(raw_headers)
-      # response headers should not have duplicates
-      @headers = raw_headers.to_h.freeze
+    def join
+      @session.succ until (@finished || @failed)
+      self
+    end
+
+    private
+    # internal: set headers and setup decoder
+    def set_headers(headers)
+      @headers = headers.freeze
+      @on_headers.call(self) if @on_headers
       @decoder = setup_decoder
     end
 
-    # @api private
-    def _chunk(encoded)
+    def add_chunk(encoded)
       chunk = @decoder.decode(encoded)
       if @on_chunk
         @on_chunk.call(chunk)
@@ -88,19 +106,16 @@ module Plum
       end
     end
 
-    # @api private
-    def _finish
+    def finish
       @finished = true
       @decoder.finish
       @on_finish.call if @on_finish
     end
 
-    # @api private
-    def _fail
-      @failed = true
+    def fail(ex = nil)
+      @failed = ex || true # FIXME
     end
 
-    private
     def setup_decoder
       if @auto_decode
         klass = Decoders::DECODERS[@headers["content-encoding"]]
diff --git a/test/plum/client/test_client.rb b/test/plum/client/test_client.rb
index 2926f82..c6c7af7 100644
--- a/test/plum/client/test_client.rb
+++ b/test/plum/client/test_client.rb
@@ -2,16 +2,6 @@ require "test_helper"
 
 using Plum::BinaryString
 class ClientTest < Minitest::Test
-  def test_request_sync
-    server_thread = start_tls_server
-    client = Client.start("127.0.0.1", LISTEN_PORT, https: true, verify_mode: OpenSSL::SSL::VERIFY_NONE)
-    res1 = client.put!("/", "aaa", headers: { "header" => "ccc" })
-    assert_equal("PUTcccaaa", res1.body)
-    client.close
-  ensure
-    server_thread.join if server_thread
-  end
-
   def test_request_async
     res2 = nil
     client = nil
@@ -42,25 +32,12 @@ class ClientTest < Minitest::Test
     server_thread.join if server_thread
   end
 
-  def test_raise_error_sync
-    client = nil
-    server_thread = start_tls_server
-    Client.start("127.0.0.1", LISTEN_PORT, https: true, verify_mode: OpenSSL::SSL::VERIFY_NONE) { |c|
-      client = c
-      assert_raises(LocalConnectionError) {
-        client.get!("/connection_error")
-      }
-    }
-  ensure
-    server_thread.join if server_thread
-  end
-
   def test_raise_error_async_seq_resume
     server_thread = start_tls_server
     client = Client.start("127.0.0.1", LISTEN_PORT, https: true, verify_mode: OpenSSL::SSL::VERIFY_NONE)
     res = client.get("/error_in_data")
     assert_raises(LocalConnectionError) {
-      client.resume(res)
+      client.resume
     }
     client.close
   ensure
@@ -82,22 +59,16 @@ class ClientTest < Minitest::Test
 
   def test_session_socket_http2_https
     sock = StringSocket.new
-    client = Client.start(sock, nil, http2: true, scheme: "https")
+    client = Client.start(sock, nil, https: true)
     assert(client.session.class == ClientSession)
   end
 
   def test_session_socket_http2_http
     sock = StringSocket.new("HTTP/1.1 100\r\n\r\n")
-    client = Client.start(sock, nil, http2: true, scheme: "http")
+    client = Client.start(sock, nil, https: false)
     assert(client.session.class == UpgradeClientSession)
   end
 
-  def test_session_socket_http1
-    sock = StringSocket.new
-    client = Client.start(sock, nil, http2: false)
-    assert(client.session.class == LegacyClientSession)
-  end
-
   private
   def start_tls_server(&block)
     ctx = OpenSSL::SSL::SSLContext.new
diff --git a/test/plum/client/test_response.rb b/test/plum/client/test_response.rb
index 511a073..a4c7fb8 100644
--- a/test/plum/client/test_response.rb
+++ b/test/plum/client/test_response.rb
@@ -3,77 +3,96 @@ require "test_helper"
 using Plum::BinaryString
 class ResponseTest < Minitest::Test
   def test_finished
-    resp = Response.new
-    resp._headers({})
+    resp = Response.new(nil)
+    resp.send(:set_headers, {})
     assert_equal(false, resp.finished?)
-    resp._finish
+    resp.send(:finish)
     assert_equal(true, resp.finished?)
   end
 
   def test_fail
-    resp = Response.new
-    resp._fail
-    assert(true, resp.failed?)
+    resp = Response.new(nil)
+    resp.send(:fail, true)
+    assert(resp.failed?, "response must be failed")
   end
 
   def test_status
-    resp = Response.new
-    resp._headers([
-      [":status", "200"]
-    ])
+    resp = Response.new(nil)
+    resp.send(:set_headers,
+      ":status" => "200"
+    )
     assert_equal("200", resp.status)
   end
 
   def test_headers
-    resp = Response.new
-    resp._headers([
-      [":status", "200"],
-      ["header", "abc"]
-    ])
+    resp = Response.new(nil)
+    resp.send(:set_headers,
+      ":status" => "200",
+      "header" => "abc"
+    )
     assert_equal("abc", resp[:HEADER])
   end
 
   def test_body
-    resp = Response.new
-    resp._headers({})
-    resp._chunk("a")
-    resp._chunk("b")
-    resp._finish
+    resp = Response.new(nil)
+    resp.send(:set_headers, {})
+    resp.send(:add_chunk, "a")
+    resp.send(:add_chunk, "b")
+    resp.send(:finish)
     assert_equal("ab", resp.body)
   end
 
   def test_body_not_finished
-    resp = Response.new
-    resp._headers({})
-    resp._chunk("a")
-    resp._chunk("b")
+    resp = Response.new(nil)
+    resp.send(:set_headers, {})
+    resp.send(:add_chunk, "a")
+    resp.send(:add_chunk, "b")
     assert_raises { # TODO
       resp.body
     }
   end
 
+  def test_on_headers_initialize
+    called = false
+    resp = Response.new(nil) { |r| called = true }
+    assert(!called)
+    resp.send(:set_headers, { ":status" => 201 })
+    assert(called)
+  end
+
+  def test_on_headers_explicit
+    called = false
+    resp = Response.new(nil)
+    resp.on_headers { |r| called = true }
+    assert(!called)
+    resp.send(:set_headers, { ":status" => 201 })
+    assert(called)
+  end
+
   def test_on_chunk
-    resp = Response.new
-    resp._headers({})
+    resp = Response.new(nil)
+    resp.send(:set_headers, {})
     res = []
-    resp._chunk("a")
-    resp._chunk("b")
-    resp._finish
+    resp.send(:add_chunk, "a")
+    resp.send(:add_chunk, "b")
+    resp.send(:finish)
     resp.on_chunk { |chunk| res << chunk }
     assert_equal(["a", "b"], res)
-    resp._chunk("c")
+    resp.send(:add_chunk, "c")
     assert_equal(["a", "b", "c"], res)
   end
 
   def test_on_finish
-    resp = Response.new
-    resp._headers({})
+    resp = Response.new(nil)
+    resp.send(:set_headers, {})
     ran = false
     resp.on_finish { ran = true }
-    resp._finish
+    resp.send(:finish)
     assert(ran)
     ran = false
     resp.on_finish { ran = true }
     assert(ran)
   end
+
+  # FIXME: test Response#join
 end