httprb / http

HTTP (The Gem! a.k.a. http.rb) - a fast Ruby HTTP client with a chainable API, streaming support, and timeouts
MIT License
3.01k stars 321 forks source link

Persistent clients don't support changing headers #725

Open byteit101 opened 2 years ago

byteit101 commented 2 years ago

Using Wireshark, I am able to confirm that this works as expected, using only one TCP connection:

 HTTP.persistent("http://api.icndb.com") {|http| 
  http.get("/jokes/random").to_s;
  http.get("/jokes/random").to_s
}

However, this doesn't, closing the TCP connection and creating a new TCP and HTTP connection for each request:

 HTTP.persistent("http://api.icndb.com") {|http| 
  http.headers("x-test" => "foo").get("/jokes/random").to_s;
  http.get("/jokes/random").to_s
}

Is there some ordering to do differently for persistent clients?

byteit101 commented 2 years ago

For ease of debugging this issue without Wireshark, I've made a statefull HTTP server for this issue:

require 'socket'

server = TCPServer.new 7182
def run(client)
    class << client
        def readish(n=1)
            begin
              result = self.read_nonblock(n)
            rescue IO::WaitReadable
              r,w,e = IO.select([self], [],[],0.2)
              if r && r.length > 0
                  retry
              else
                  nil
              end
            rescue IOError
                nil
            end
        end
    end
    puts "accepted"
    # first url
    s = ""
    until s.include? "\r\n\r\n"
        tmp = client.readish(1)
        if tmp.nil?
            puts "Closed in the middle 1, errror!"
            client.close
            return
        end
        s << tmp
    end
    if s.match(/^(GET|get) \/start/) == nil
        puts "wrong start!"
        client.puts "HTTP/1.1 400 WrongStart\r\n\r\nBAD"
        client.close
        return
    end
    puts "Good start"
    client.puts "HTTP/1.1 200 Ok\r\nKeep-Alive: timeout=5, max=100\r\nConnection: Keep-Alive\r\ncontent-Length: 1\r\n\r\nG"

    # second url
    puts "On to next"
    s = ""
    until s.include? "\r\n\r\n"
        tmp = client.readish(1)
        if tmp.nil?
            puts "Closed in the middle3, errror!"
            client.puts "HTTP/1.1 467 YouClosedIt\r\n\r\nBAD"
            client.close
            return
        end
        s << tmp
    end
    if s.match(/^(GET|get) \/finish/) == nil
        client.puts "HTTP/1.1 400 WrongFinish\r\n\r\nBAD"
        client.close
        return
    end
    puts "Good End!"
    client.puts "HTTP/1.1 200 OK\r\nConnection: Close\r\ncontent-Length: 3\r\n\r\n:-)"
    client.close    
end

loop do
    run(server.accept)
end

Correct Persistence (expected)

2.6.2 :046 > HTTP.persistent("http://localhost:7182") {|http| http.get("/start").to_s; puts http.get("/finish").to_s }
:-)
 => nil 

Incorrect Persistence (actual)

2.6.2 :047 > HTTP.persistent("http://localhost:7182") {|http| http.headers("x-test" => "foo").get("/start").to_s; puts http.get("/finish").to_s }
BAD
 => nil 
tarcieri commented 2 years ago

While this is annoying and weird behavior, I think you can work around it by passing headers as an option to get

byteit101 commented 2 years ago

Ah yes, that appears to have worked. Should put the workaround on the wiki page until it's fixed.