proof of concept: split ProtocolError into socket.connect, socket.write, socket.read

[kevinburke]

In #547 I said we could detect which type of error got raised by httplib. Here is a proof of concept showing how to do this.

The essential httplib calls are both in _make_request: the first is conn.request and the second is conn.getresponse(). In order to distinguish these at the urlopen level, I had to split _make_request into two functions. Otherwise, you have to distinguish between errors between all three of socket connect/write/read; did the ETIMEDOUT occur on the connect or the read?

• This gets ugly since we want to pass a lot of state from the first function to the second function, in particular the conn object and the timeout. There's a failing test test_total_timeout that I couldn't figure out what to do with (besides bail on the feature, which I wouldn't be too against)
• a lot of the tests needed to be updated to call both _make_ functions, which shouldn't be too terrible, since it's an underscored function.
• what is terrible is most of the exception handling has to be duplicated for _make_connect and _make_request... some of it could probably be killed if we looked harder at what could be thrown by both methods.

So yeah, I'm not amazingly sure about this, and given that I feel bad about code I felt better about when I submitted it, I'm not sure this is worth going down.

Still, this is evidence that if we wanted to do this, we could without necessarily rewriting httplib.

[shazow]

Could you tell us more about the tests that are failing and while you believe they might be failing?

Also do you have any tests which show that this captures a practical distinction which was not captured before?

[shazow]

Could you tell us more about the tests that are failing and while you believe they might be failing?

Also do you have any tests which show that this captures a practical distinction which was not captured before?

[kevinburke]

will do, prob won't be till late tonight though...

[kevinburke]

will do, prob won't be till late tonight though...

[kevinburke]

Say a socket is closed for ~0.05 seconds when a server or HAProxy restarts (a case we saw multiple times a day at Twilio). If you write

# Read=0 since this is billing code or whatever and we don't want to try twice
r = Retry(connect=3, read=0)
pool = HTTPConnectionPool('localhost', 8722)
pool.request('POST', '/charge-the-credit-card', retries=r)

In master this will fail with a MaxRetryError(ProtocolError("Connection refused")). The connection refused error counts as a read error and is not retried. But it's clearly safe to retry, the remote socket wasn't open. This change properly catches those as connection errors.

(I've been thinking about how to test this, and I'm not sure because you want the first two or three tries to fail with a connection refused and then you want to open the socket for the last try. Coordinating this in a test would be tricky).

[kevinburke]

Say a socket is closed for ~0.05 seconds when a server or HAProxy restarts (a case we saw multiple times a day at Twilio). If you write

# Read=0 since this is billing code or whatever and we don't want to try twice
r = Retry(connect=3, read=0)
pool = HTTPConnectionPool('localhost', 8722)
pool.request('POST', '/charge-the-credit-card', retries=r)

In master this will fail with a MaxRetryError(ProtocolError("Connection refused")). The connection refused error counts as a read error and is not retried. But it's clearly safe to retry, the remote socket wasn't open. This change properly catches those as connection errors.

(I've been thinking about how to test this, and I'm not sure because you want the first two or three tries to fail with a connection refused and then you want to open the socket for the last try. Coordinating this in a test would be tricky).

[kevinburke]

The tests are failing because _make_connect sets the connection timeout and the starting time, then _make_request clones the timeout object that's passed in, which wipes the observed connect starting time. When these were one function, we could use the same timeout object all the way through.

I'm not sure about the pool_size function, I'll look at it.

[kevinburke]

The tests are failing because _make_connect sets the connection timeout and the starting time, then _make_request clones the timeout object that's passed in, which wipes the observed connect starting time. When these were one function, we could use the same timeout object all the way through.

I'm not sure about the pool_size function, I'll look at it.

[kevinburke]

(A DNS error would fall in the same boat of connection errors that are caught as read errors)

[kevinburke]

(A DNS error would fall in the same boat of connection errors that are caught as read errors)

[shazow]

We already have tests that do things like this (though I usually cut the retries down to 2). E.g.
https://github.com/shazow/urllib3/blob/master/test/with_dummyserver/test_socketlevel.py#L79
https://github.com/shazow/urllib3/blob/master/test/with_dummyserver/test_socketlevel.py#L256

Bunch others too, look around in that file.

Also keep in mind we're raising some custom socket.timeout errors in our contrib.pyopenssl, not sure if that will mess with things or how we change the raises to not mess with things.

[shazow]

We already have tests that do things like this (though I usually cut the retries down to 2). E.g.
https://github.com/shazow/urllib3/blob/master/test/with_dummyserver/test_socketlevel.py#L79
https://github.com/shazow/urllib3/blob/master/test/with_dummyserver/test_socketlevel.py#L256

Bunch others too, look around in that file.

Also keep in mind we're raising some custom socket.timeout errors in our contrib.pyopenssl, not sure if that will mess with things or how we change the raises to not mess with things.

[theacodes]

Thank you for creating this pull request, unfortunately, for one reason or
another it has become stale. We are declaring PR bankruptcy (see #1370) to
better focus our limited volunteer resources. If you still think this PR is
relevant and useful, please comment and let us know!

[theacodes]

Thank you for creating this pull request, unfortunately, for one reason or
another it has become stale. We are declaring PR bankruptcy (see #1370) to
better focus our limited volunteer resources. If you still think this PR is
relevant and useful, please comment and let us know!