Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

couldn't read response headers (HTTP::ConnectionError) #459

Open
tycooon opened this issue Feb 15, 2018 · 11 comments
Open

couldn't read response headers (HTTP::ConnectionError) #459

tycooon opened this issue Feb 15, 2018 · 11 comments
Labels

Comments

@tycooon
Copy link
Contributor

tycooon commented Feb 15, 2018

I get this error when using persistent connection with high keep_alive_timeout value.

Test script:

require "http"

url = "https://www.google.com/"
client = HTTP.persistent(url, timeout: 1000)

puts Time.now
puts client.get(url).to_s.size
sleep 5
puts client.get(url).to_s.size
sleep 240
puts client.get(url).to_s.size

This consistently gives me the following output:

15551
15594
Traceback (most recent call last):
	6: from tmp/test-http.rb:28:in `<main>'
	5: from /Users/tycooon/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/http-3.0.0/lib/http/chainable.rb:20:in `get'
	4: from /Users/tycooon/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/http-3.0.0/lib/http/client.rb:43:in `request'
	3: from /Users/tycooon/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/http-3.0.0/lib/http/client.rb:66:in `perform'
	2: from /Users/tycooon/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/http-3.0.0/lib/http/connection.rb:99:in `read_headers!'
	1: from /Users/tycooon/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/http-3.0.0/lib/http/connection.rb:99:in `loop'
/Users/tycooon/.rbenv/versions/2.5.0/lib/ruby/gems/2.5.0/gems/http-3.0.0/lib/http/connection.rb:101:in `block in read_headers!': couldn't read response headers (HTTP::ConnectionError)
@ixti ixti added the Bug label Feb 15, 2018
@britishtea
Copy link
Contributor

I think #420 is related.

@tycooon
Copy link
Contributor Author

tycooon commented Feb 15, 2018

I checked how httpclient gem works and it looks like it just reconnects and retries the request:

require "httpclient"
require "httplog"

HttpLog.configure do |config|
  config.logger = Logger.new(STDOUT)
  config.log_response = false
end

url = "https://www.google.com/"
client = HTTPClient.new
client.keep_alive_timeout = 1000

puts Time.now
puts client.get(url).body.to_s.size
sleep 5
puts client.get(url).body.to_s.size
sleep 240
puts client.get(url).body.to_s.size
D, [2018-02-15T19:13:55.099962 #88760] DEBUG -- : [httplog] Sending: GET https://www.google.com/
D, [2018-02-15T19:13:55.100057 #88760] DEBUG -- : [httplog] Data:
D, [2018-02-15T19:13:55.100245 #88760] DEBUG -- : [httplog] Connecting: www.google.com:443
D, [2018-02-15T19:13:55.454928 #88760] DEBUG -- : [httplog] Status: 200
D, [2018-02-15T19:13:55.455023 #88760] DEBUG -- : [httplog] Benchmark: 0.3291 seconds
15584
Cookie#domain returns dot-less domain name now. Use Cookie#dot_domain if you need "." at the beginning.
D, [2018-02-15T19:14:00.459097 #88760] DEBUG -- : [httplog] Sending: GET https://www.google.com/
D, [2018-02-15T19:14:00.459185 #88760] DEBUG -- : [httplog] Data:
D, [2018-02-15T19:14:00.570022 #88760] DEBUG -- : [httplog] Status: 200
D, [2018-02-15T19:14:00.570082 #88760] DEBUG -- : [httplog] Benchmark: 0.110737 seconds
15580
D, [2018-02-15T19:18:00.575761 #88760] DEBUG -- : [httplog] Sending: GET https://www.google.com/
D, [2018-02-15T19:18:00.575906 #88760] DEBUG -- : [httplog] Data:
D, [2018-02-15T19:18:00.576904 #88760] DEBUG -- : [httplog] Sending: GET https://www.google.com/
D, [2018-02-15T19:18:00.576980 #88760] DEBUG -- : [httplog] Data:
D, [2018-02-15T19:18:00.577129 #88760] DEBUG -- : [httplog] Connecting: www.google.com:443
D, [2018-02-15T19:18:00.884618 #88760] DEBUG -- : [httplog] Status: 200
D, [2018-02-15T19:18:00.884695 #88760] DEBUG -- : [httplog] Benchmark: 0.307552 seconds
15584

@tycooon
Copy link
Contributor Author

tycooon commented Feb 16, 2018

So I checked how httpclient is handling this case and it raises the KeepAliveDisconnected error in case when socket.gets returns nil: https://github.com/nahi/httpclient/blob/27fbb07685930d1c8dbdc8dd1e72027f79879bbe/lib/httpclient/session.rb#L808. They always retry on this error once: https://github.com/nahi/httpclient/blob/27fbb07685930d1c8dbdc8dd1e72027f79879bbe/lib/httpclient.rb#L1131

I also checked this against my server and in this case the request does not reach the server (doesn't appear in nginx access log) so it's safe to retry it.

And by the way, I also didn't find any retrying for errors like Errno::ECONNABORTED or Errno::ECONNRESET. Does this mean that it's up to user to retry on all these errors?

@tarcieri
Copy link
Member

Per what @britishtea noted this appears to be a dupe of #420.

This library has no feature for automatic retries. That would be useful, but should probably be opened as a separate issue.

The root cause appears to be the connection timing out, although I am confused (as in #420) why this occurs as an error reading the response as opposed to sending the request.

Regarding this:

And by the way, I also didn't find any retrying for errors like Errno::ECONNABORTED or Errno::ECONNRESET. Does this mean that it's up to user to retry on all these errors?

Again, there's no automatic retry support, however these sorts of errors are collectively rescued as SystemCallError and re-raised as HTTP::ConnectionError, the latter being what your code needs to handle. See:

https://github.com/httprb/http/blob/master/lib/http/request/writer.rb#L103
https://github.com/httprb/http/blob/master/lib/http/connection.rb#L219

@tycooon
Copy link
Contributor Author

tycooon commented Feb 22, 2018

these sorts of errors are collectively rescued as SystemCallError and re-raised as HTTP::ConnectionError, the latter being what your code needs to handle

The problem is, HTTP::ConnectionError can be raised because of all sorts of errors and it's hard to tell which of them are always safe to retry and which are not – there is no link to the original exception at least.

In my opinion, if the library provides persistent connections support, it should either handle the retrying itself or provide some relevant errors so that client can figure out what to do.

@zanker
Copy link
Contributor

zanker commented Feb 22, 2018 via email

@tarcieri
Copy link
Member

HTTP::ConnectionError can be raised because of all sorts of errors and it's hard to tell which of them are always safe to retry and which are not – there is no link to the original exception at least.

All of these errors indicate an error within the request lifecycle. Whether they should be retried is more a question of the request being performed, not the specific error that occurred.

If you have a counterexample where depending on the specific error that occurred, you'd retry it in one case but not another, I'd love to hear it.

In my own observations of e.g. browser retry logic, GET requests are repeatedly retried even if the error which is occurring is changing, and these retries continue until the page loads successfully or some max retry threshold is reached.

In my opinion, if the library provides persistent connections support, it should either handle the retrying itself or provide some relevant errors so that client can figure out what to do.

Then please open an issue describing the specific feature you'd like added.

@jrochkind
Copy link
Contributor

jrochkind commented Jul 5, 2018

In my own observations of e.g. browser retry logic, GET requests are repeatedly retried even if the error which is occurring is changing, and these retries continue until the page loads successfully or some max retry threshold is reached.

Yep, this is because "GET" is defined as both idempotent and "safe". https://codeahoy.com/2016/06/30/idempotent-and-safe-http-methods-why-do-they-matter/

Whether the request should be retried in general probably depends on the HTTP method. In general, i think it would be not a bad feature for http.rb to be optionally able to automatically retry appropriate methods.

But I'm not convinced that persistent connections isn't a special case. If you are using persistent connections, the connection the client thought was still open in fact being dropped by the server is probably something you expect to run into, regularly. If it's possible to catch this case (and I'm not sure it is), I think it would be appropriate for http.rb to automatically re-establish connection in all cases, without it even being an option, without regard to the HTTP method in question. if it's possible to tell that the connection had been dropped and the request definitely never made it to the server -- not sure if it is, but this is a special case relevant to persistent connections.

I thin browsers must do something special here, because browsers will not retry POST in the general case; browsers DO use persistent connections; and browsers are not always complaining "Oh, we thought it was a persistent connection but it got dropped but it was a non-idempotent POST so we can't safely retry it, sorry you're out of luck".

@cpb
Copy link

cpb commented Nov 2, 2018

I'm noticing this when the server signals to close the connection, and I found this resolved things for me:

    class RespectTheClose < Http::Feature
      def wrap_response(response)
        case response[:connection] when /close/i then
          response.flush.connection.close
        end

        response
      end

      Http::Options.register_feature(:respect_the_close,self)
    end

Do the response headers include a Connection header?

@ixti
Copy link
Member

ixti commented Nov 2, 2018

Yes. Server might respond with Connection: close header to notify client that it wants to close connection. And I believe this should be handled by core.

@froodian
Copy link

froodian commented Sep 20, 2023

For the scenario where the server sent the Connection: close header in a previous response, an updated version of the workaround above:

module HTTP
  module Features
    class RespectConnectionCloseResponseHeader < HTTP::Feature
      def wrap_response(response)
        if response[:connection]&.casecmp("close") == 0
          response.flush.connection.close
        end
        return response
      end
      HTTP::Options.register_feature(:respect_connection_close_response_header, self)
    end
  end
end

and then at the usage site, instead of HTTP.persistent(url), use

HTTP.use(:respect_connection_close_response_header).persistent(url)

In terms of why that is happening, and maybe getting the issue fixed in the library, this appears to be necessary because this line of code is trying to call close on the connection unless keep_alive?, but that keep_alive? check is only checking whether the request headers specified keep-alive or close, and nothing checks whether the response headers specified keep-alive or close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants