Although HTTP/1.1 says that servers SHOULD send a Connection: close
header if they intend to close a persistent connection (sec 8.1.2.1),
clients (like httplib) "MUST be able to recover from asynchronous close
events. Client software SHOULD reopen the transport connection and
retransmit the aborted sequence of requests without user interaction so
long as the request sequence is idempotent" (sec 8.1.4) since servers
MAY close a persistent connection after a request due to a timeout or
other reason. Further, "Clients and servers SHOULD both constantly
watch for the other side of the transport close, and respond to it as
appropriate." (sec 8.1.4).

httplib currently does not detect when the server closes its side of the
connection, until the following bit of HTTPResponse in httplib.py
(python2.5.2):

    def _read_status(self):
        # Initialize with Simple-Response defaults
        line = self.fp.readline()
        ...
        if not line:
            # Presumably, the server closed the connection before
            # sending a valid response.
            raise BadStatusLine(line)
        ...

So we end up raising a BadStatusLine exception for a completely
permissible case: the server closed a persistent connection. This
causes persistent connections to fail for users of httplib in mysterious
ways, especially if they are behind proxies: Squid, for example, seems
to limit persistent connections to a maximum of 3 requests, and then
closes the connection, causing future requests to raise the BadStatusLine.

There appears to be code attempting to fix this problem in
HTTPConnection.request(), but it doesn't always work. RFC793 says, "If
an unsolicited FIN arrives from the network, the receiving TCP can ACK
it and tell the user that the connection is closing. The user will
respond with a CLOSE, upon which the TCP can send a FIN to the other TCP
after sending any remaining data." (sec 3.5 case 2) Key phrase here is
"after sending any remaining data": python is usually allowed to put the
request on the network without raising a socket.error; the close is not
signaled to python until HTTPResponse.begin() invokes
HTTPResponse._read_status.

It would be best to extend the retry logic in request() to cover this
case as well (store away the previous parameters to request() and if
_read_status() fails invoke HTTPConnection.close(),
HTTPConnection.connect(), HTTPConnection.request(stored_params), and
retry the HTTPConnection.getresponse(). But at the very least python
should document and raise a EAGAIN exception of some kind so that the
caller can distinguish this case from an actual bad status line and
retry the request.

FWIW, the following code works around the issue for me:

class HTTPConnection(httplib.HTTPConnection):
    def request(self, method, url, body=None, headers={}):
        # patch upstream to recv() after we send, so that a closed
        # connection is discovered here rather than later in
        # HTTPResponse._read_status()
        try:
            self._send_request(method, url, body, headers)
            b = self.sock.recv(1, socket.MSG_PEEK)
            if b == '':
                self.close()
                raise socket.error(32) # sender closed connection.
        except socket.error, v:
            # trap 'Broken pipe' if we're allowed to automatically reconnect
            if v[0] != 32 or not self.auto_open:
                raise
            # try one more time
            self._send_request(method, url, body, headers)

In general, attempting to read a byte of the response just to trigger
the broken pipe error is suboptimal; it doesn't accurately distinguish
the "closed before received request" case from the "closed just after
received request" case. For idempotent methods like GET and HEAD, this
doesn't matter, but it does for POST, etc.

There ought to be a better way to force the underlying OS to give you
the current 'closed' status on a socket which doesn't require your
trying to send/receive bytes. The fundamental issue here is that the
TCP spec, and Linux, allow the request send to succeed even if TCP FIN
has been received from the other side -- the receipt of FIN after the
response is received is always going to be a race to some degree.

All of which is why the HTTP pipelining specs encourage you *not* to use
pipelining for non-idempotent requests. The above patch could be
tweaked to only perform the MSG_PEEK and retry if this is the second or
later pipelined request. Then, following the HTTP recommendations and
always calling HttpConnection.close()/HttpConnection.connect() before
invoking "unsafe" requests like POST, would make the whole thing safe.

btw, when using async io (poll, select, etc) I -think- your socket will
see a read event when the server closes the connection (sends you a FIN
or even a RST) at which point your sock.recv() when you've been told
data was ready will return 0 bytes indicating that the server has closed
the connection.

I like your MSG_PEEK hack here. I'll figure out if this or something
else should go into (the giant mess that is) httplib.

Not that we've removed the try one more time branch of the code,
because it was causing other problems.

Jeremy

On Sun, Aug 16, 2009 at 6:24 PM, Gregory P. Smith<report@bugs.python.org> wrote:

Gregory P. Smith <greg@krypto.org> added the comment:

btw, when using async io (poll, select, etc) I -think- your socket will
see a read event when the server closes the connection (sends you a FIN
or even a RST) at which point your sock.recv() when you've been told
data was ready will return 0 bytes indicating that the server has closed
the connection.

I like your MSG_PEEK hack here.  I'll figure out if this or something
else should go into (the giant mess that is) httplib.

----------
priority:  -> normal

---

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue3566\>

---

---

Python-bugs-list mailing list
Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/jeremy%40alum.mit.edu

unassigning, i don't had time to look at this one.

Seems to affect 2.7 too.

I don’t think the peek hack needs to go into to the standard library. It is just a workaround for code using the current library, to differentiate an empty status line due to EOF from any other kind of “bad” status line. This is what bpo-8450, “False BadStatusLine” is about.

Trying to reproduce this on my own in 3.5, 2.7 and 2.5 yields a ConnectionResetError (ECONNRESET), which seems to be correct. That said, this could be due to varying TCP implementations on the server so might still be valid. It could also be due to an older kernel which has been fixed since this was originally reported. Is this still reproducible? If so, can an example be provided?

If the error as written is reproducible, I think that the error message should be fixed, but I'm not so sure that any more than that should be done.

As far as the RFC goes, I think the MUST clause pointed out can be left to the interpretation of the reader. You /could/ consider http.client as the client, but you could also consider the application that a user is interacting with as the client.

Offhand, I think that the library as it is does the right thing in allowing the application code to handle the exceptions as it sees fit. Because http.client in its current state doesn't allow for request pipelining, retries from calling code should be trivial, if that is what the caller intends to do.

I believe the BadStatusLine can still happen, depending on the circumstances. When I get a chance I will see if I can make a demonstration. In the meantime these comments from my persistent connection handler <https://github.com/vadmium/python-iview/blob/587a198/iview/utils.py#L169\> might be useful:

# If the server closed the connection,
# by calling close() or shutdown(SHUT_WR),
# before receiving a short request (<= 1 MB),
# the "http.client" module raises a BadStatusLine exception.

# To produce EPIPE:
# 1. server: close() or shutdown(SHUT_RDWR)
# 2. client: send(large request >> 1 MB)

# ENOTCONN probably not possible with current Python,
# but could be generated on Linux by:
# 1. server: close() or shutdown(SHUT_RDWR)
# 2. client: send(finite data)
# 3. client: shutdown()
# ENOTCONN not covered by ConnectionError even in Python 3.3.

# To produce ECONNRESET:
# 1. client: send(finite data)
# 2. server: close() without reading all data
# 3. client: send()

I think these behaviours were from experimenting on Linux with Python 3 sockets, and reading the man pages.

I think there should be a new documented exception, a subclass of BadStatusLine for backwards compatibility. Then user code could catch the new exception, and true bad status lines that do not conform to the specification or whatever won’t be caught. I agree that the library shouldn’t be doing any special retrying of its own, but should make it easy for the caller to do so.

Okay here is a demonstration script, which does two tests: a short basic GET request, and a 2 MB POST request. Output for me is usually:

Platform: Linux-3.15.5-2-ARCH-x86_64-with-arch
Normal request: getresponse() raised BadStatusLine("''",)
2 MB request: request() raised BrokenPipeError(32, 'Broken pipe'); errno=EPIPE

Sometimes I get a BadStatusLine even for the second request:

Platform: Linux-3.15.5-2-ARCH-x86_64-with-arch
Normal request: getresponse() raised BadStatusLine("''",)
2 MB request: getresponse() raised BadStatusLine("''",)

Spotted code in Python’s own library that maintains a persistent connection and works around this issue:

Lib/xmlrpc/client.py:1142

Hi Martin,

Thanks for the example code. I'm not sure whether or not this was your intention, but your example demonstrates a misbehaving client, one that seems to expect a persistent connection from a non-persistent server. TCPServer will only serve a single request and will shutdown and close the socket once the response has been written.

The reason that you're seeing different responses sometimes (varying between BadStatusLine and BrokenPipeError) is because of an understandable race condition between the client sending the requests and the server fully shutting down the socket and the client receiving FIN.

After digging into this, I'm not sure that there is a better way of handling this case. This exception can occur whether the client has issued a request prior to cleaning up and is expecting a response, or the server is simply misbehaving and sends an invalid status line (i.e. change your response code to an empty string to see what I mean).

I'll do some further digging, but I don't believe that there's really a good way to determine whether the BadStatusLine is due to a misbehaving server (sending a non-conforming response) or a closed socket. Considering that the client potentially has no way of knowing whether or not a server socket has been closed (in the case of TCPServer, it does a SHUT_WR), I think that BadStatusLine may be the appropriate exception to use here and the resulting action would have to be left up to the client implementation, such as in xmlrpc.client.

I'm not sure whether or not this was your intention, but your example demonstrates a misbehaving client, one that seems to expect a persistent connection from a non-persistent server. TCPServer will only serve a single request and will shutdown and close the socket once the response has been written.

Sorry, I mis-worded that. I'm /assuming/ that the misbehaving client is what you were intending on demonstrating as it shows the server closing the connection before the client expects it to do so.

Hi Demian, my intention is to demonstrate normal usage of Python’s HTTP client, whether or not its implementation misbehaves. I am trying to demonstrate a valid persistent server that happens to decide to close the connection after the first request but before reading a second request. Quoting the original post: “Servers may close a persistent connection after a request due to a timeout or other reason.” I am attaching a second demo script which includes short sleep() calls to emulate a period of time elapsing and the server timing out the connection, which is common for real-world servers.

The new script also avoids the EPIPE race by waiting until the server has definitely shut down the socket, and also demonstrates ECONNRESET. However this synchronization is artificial: in the real world the particular failure mode (BadStatusLine/EPIPE/ECONNRESET) may be uncertain.

If you are worried about detecting a misbehaving server that closes the connection before even responding to the first request, perhaps the HTTPConnection class could maintain a flag and handle the closed connection differently if it has not already seen a complete response.

If you are worried about detecting a misbehaving server that sends an empty status line without closing the connection, there will still be a newline code received. This is already handled separately by existing code: Lib/http/client.py:210 versus Lib/http/client.py:223.

I think there should be a separate exception, say called ConnectionClosed, for when the “status line” is an empty string (""), which is caused by reading the end of the stream. This is valid HTTP behaviour for the second and subsequent requests, so the HTTP library should understand it. BadStatusLine is documented for “status codes we don’t understand”. The new ConnectionClosed exception should probably be a subclass of BadStatusLine for backwards compatibility.

A further enhancement could be to wrap any ConnectionError during the request() stage, or first part of the getresponse() stage, in the same ConnectionClosed exception. Alternatively, the new ConnectionClosed exception could subclass both BadStatusLine and ConnectionError. Either way, code like the XML-RPC client could be simplified to:

try:
return self.single_request(...)
except http.client.ConnectionClosed:
#except ConnectionError: # Alternative
#retry request once if cached connection has gone cold
return self.single_request(...)

FWIW, I agree with the analysis here, its standard HTTP behaviour in the real world, and we should indeed handle it.

Sorry Martin, I should really not dig into issues like this first thing in the morning ;)

My concern about the proposed change isn't whether or not it isn't valid HTTP behaviour, it is. My concern (albeit a small one) is that the change implies an assumption that may not necessarily be true. No matter how valid based on the HTTP spec, it's still an assumption that /can/ potentially lead to confusion. I do agree that a change /should/ be made, I just want to make sure that all potential cases are considered before implementing one.

For example, applying the following patch to the first attachment:

52,57c52,53
< self.wfile.write(
< b"HTTP/1.1 200 Dropping connection\r\n"
< b"Content-Length: 0\r\n"
< b"\r\n"
< )
<
---

    self.wfile.write(b'')

Should the proposed change be made, the above would error out with a ConnectionClosed exception, which is invalid because the connection has not actually been closed and BadStatusLine is actually closer to being correct. Admittedly, this is a little contrived, doesn't adhere to the HTTP spec and is much less likely to happen in the real world than a connection unexpectedly closed by the server, but I don't think it's an unreasonable assumption for lesser used servers or proxies or those in development. In those cases, the proposed change would result in just as much confusion as the current behaviour with connections that are intended to be persistent.

In my mind, the one constant through both of these cases is that the response that the client has read is unexpected. In light of that, rather than a ConnectionClosed error, what about UnexpectedResponse, inheriting from BadStatusLine for backwards compatibility and documented as possibly being raised as a result of either case? I think this would cover both cases where a socket has been closed as well as an altogether invalid response.

Calling self.wfile.write(b"") should be equivalent to not calling write() at all, as far as I understand. Using strace, it does not seem to invoke send() at all. So the result will depend on what is written next. In the case of my code, nothing is written next; the connection is shut down instead. So I don’t believe this case is any different from “a connection unexpectedly closed by the server”. To be clear, I think the situation we are talking about is:

1. Client connects to server and sends short request; server accepts connection and possibly reads request
2. Server does not write any response, or just calls write(b""), which is equivalent
3. Server shuts down connection
4. Client reads end of stream (b"") instead of proper status line

But to address your concern in any case, see the third paragram in <https://bugs.python.org/issue3566#msg234330\>. I propose some internal flag like HTTPConnection._initial_response, that gets set to False after the first proper response is received. Then the code could be changed to something like:

if not line:
    # Presumably, the server closed the connection before
    # sending a valid response.
    if self._initial_response:
        raise BadStatusLine(line)
    else:
        raise ConnectionClosed("Stream ended before receiving response")

Calling self.wfile.write(b"") should be equivalent to not calling write() at all, as far as I understand.

Right (or at least, as I understand it as well).

Really, this boils down to a philosophical debate: Should the standard library account for unexpected conditions where possible (within reason of course), or should it only account for conditions as described by specifications?

1. Client connects to server and sends short request; server accepts connection and possibly reads request
   [snip]

This flow makes sense and is well accounted for with your proposed change. However, misbehaving cases such as:

1. Client connects to server or proxy (possibly through a tunnel) and sends request; server/proxy accepts connection and possibly reads request
2. Server/proxy intends to send a response, but doesn't for any number of reasons (bug, still in development, etc)
3. The connection is not closed and subsequent requests may succeed

Granted, the result is unexpected and doesn't comply with HTTP RFCs. However, leading the user to believe that the connection has been closed when it actually hasn't is misleading. I've spent many an hour trying to hunt down root causes of issues like this and bashed my head against a keyboard in disbelief when I found out what the cause /really/ was. Because of those headaches, I still think that the introduction of an UnexpectedResponse, if well documented, covers both cases nicely, but won't heatedly argue it further :) If others (namely core devs) think that the introduction of ConnectionClosed exception is a better way to go, then I'll bow out. It would maybe be nice to have Senthil chime in on this.

But to address your concern in any case, see the third paragram in <https://bugs.python.org/issue3566#msg234330\>.

I don't think that should be added at all as the issue that I'm describing can occur at any point, not only the first request.

On another note, were you interested in submitting a patch for this?

Yeah I’m happy to put a patch together, once I have an idea of the details.

I’d also like to understand your scenario that would mislead the user to believe that the connection has been closed when it actually hasn’t. Can you give a concrete example or demonstration?

Given your misbehaving flow:

1. Client connects to server or proxy (possibly through a tunnel) and sends request; server/proxy accepts connection and possibly reads request
2. Server/proxy intends to send a response, but doesn't for any number of reasons (bug, still in development, etc)
3. The connection is not closed and subsequent requests may succeed

I would expect the client would still be waiting to read the status line of the response that was never sent in step 2. Eventually the server _would_ probably drop the connection (so ConnectionClosed would not be misleading), or the client would time it out (a different error would be raised).

I think that in other stdlib networking modules, a connection closed error is raised when an operation is attempted on a closed connection. For example, in smtplib, the server may return an error code and then (contrary to the RFC) close the connection. We fixed a bug in smtplib where this was mishandled (the error code was lost and SMTPServerDisconnected was raised instead). Now we return the error code, and the *next* operation on the connection gets the connection closed error.

I think this is a good model, but I'm not sure if/how it can be applied here. That is, if the connection has been closed, I think an operation attempted on the connection should raise an exception that makes it clear that the connection is closed (in the case of some stdlib modules, that may be a socket level exception for the operation on the closed socket).

I think the remote server writing a blank line to the socket is a very different thing from the remote server closing the connection without writing anything, so I may be misunderstanding something here. Note that handling this is potentially more complicated with https, since in that case we have a wrapper around the socket communication that has some buffering involved.

But also note that if a new exception is introduced this becomes a feature and by our normal policies can only go into 3.5.

TL;DR: Because HTTP is an application-level protocol, it's nearly impossible to gauge how a server will behave given a set of conditions. Because of that, any time that assumptions can be avoided, they should be.

@r. David Murray:

That is, if the connection has been closed, I think an operation attempted on the connection should raise an exception that makes it clear that the connection is closed (in the case of some stdlib modules, that may be a socket level exception for the operation on the closed socket).

In the typical case, this is exactly what happens. This issue is around a race condition that can occur between the client issuing a request prior to terminating the connection with the server, but the server terminating it prior to processing the request. In these cases, the client is left in a state where as far as it's concerned, it's in a valid state waiting for a response which the server will not issue as it has closed the socket on its side. In this case, the client reading an empty string from the receive buffer implies a closed socket. Unfortunately, it's not entirely uncommon when using persistent connections, as Martin's examples demonstrate.

I think the remote server writing a blank line to the socket is a very different thing from the remote server closing the connection without writing anything, so I may be misunderstanding something here.

+1. What Martin is arguing here (Martin, please correct me if I'm wrong) is that a decently behaved server should not, at /any/ time write a blank line to (or effectively no-op) the socket, other than in the case where the socket connection has been closed. While I agree in the typical case, a blend of Postel and Murphy's laws leads me to believe it would be better to expect, accept and handle the unexpected.

@martin:

Here's a concrete example of the unexpected behaviour. It's not specific to persistent connections and would be caught by the proposed "first request" solution, but ultimately, similar behaviour may be seen at any time from other servers/sources: http://stackoverflow.com/questions/22813645/options-http-methods-gives-an-empty-response-on-heroku.

Googling for "http empty response" and similar search strings should also provide a number of examples where unexpected behaviour is encountered and in which case raising an explicit "ConnectionClosed" error would add to the confusion.

Other examples are really hypotheticals and I don't think it's worth digging into them too deeply here. Unexpected behaviour (regardless of whether it's on the first or Nth request) should be captured well enough by now.

Eventually the server _would_ probably drop the connection (so ConnectionClosed would not be misleading)

Sure, but you're raising an exception based on future /expected/ behaviour. That's my issue with the proposed solution in general. ConnectionClosed assumes specific behaviour, where literally /anything/ can happen server side.

Now I think I'd like to take my foot out of my mouth.

Previous quick experiments that I had done were at the socket level, circumventing some of the logic in the HTTPResponse, mainly the calls to readline() rather than simple socket.recv(<N>).

I've confirmed that the /only/ way that the HTTPConnection object can possibly get a 0-length read is, in fact, if the remote host has closed the connection.

In light of that, I have no objection at all to the suggested addition of ConnectionClosed exception and my apologies for the added confusion and dragging this issue on much longer than it should have been.

I've also attached my proof of concept code for posterity.

(Admittedly, I may also have been doing something entirely invalid in previous experiments as well)

Here is a patch, including tests and documentation. It ended up a bit more complicated than I anticipated, so I’m interested in hearing other ideas or options.

• Added http.client.ConnectionClosed exception
• HTTPConnection.close() is implicitly called for a persistent connection closure
• BadStatusLine or ConnectionError (rather than new exception) is still raised on first getresponse()
• request() raising a ConnectionError does not necessarily mean the server did not send a response, so ConnectionClosed is only raised by getresponse()
• ConnectionClosed wraps ECONNRESET from the first recv() of the status line, but not after part of the status line has already been received

With this I hope code for making idempotent requests on a persistent connection would look a bit like this:

def idempotent_request(connection)
    try:
        attempt_request(connection, ...)
        response = connection.get_response()
        if response.status == HTTPStatus.REQUEST_TIMEOUT:
            raise ConnectionClosed(response.reason)
    except ConnectionClosed:
        attempt_request(connection, ...)
        response = connection.get_response()
    return response

def attempt_request(connection):
    try:
        connection.request(...)
    except ConnectionError:
        pass  # Ignore and read server response

Updated v2 patch. This version avoids intruding into the HTTPConnection.connect() implementation, so that users, tests, etc may still set the undocumented “sock” attribute without calling the base connect() method. Also code style changes based on feedback to another of my patches.

Thanks for the patch Martin (as well as catching a couple issues that I'd run into recently as well). I've left a couple comments up on Rietveld.

Thanks for the reviews.

I agree about the new HTTPResponse flag being a bit awkward; the HTTPResponse class should probably raise the ConnectionClosed exception in all cases. I was wondering if the the HTTPConnection class should wrap this in a PersistentConnectionClosed exception or something if it wasn’t for the first connection, now I’m thinking that should be up to the higher level user, in case they are doing something like HTTP preconnection.

now I’m thinking that should be up to the higher level user

+1. A closed connection is a closed connection, whether it's persistent or not. The higher level code should be responsible for the context, not the connection level.

I have changed my opinion of the “peek hack” from <https://bugs.python.org/issue3566#msg231413\>. It would be useful when doing non-idempotent requests like POST, to avoid sending a request when we know it is going to fail. I looked into how to implement it so that it works for SSL (which does not support MSG_PEEK), and the neatest solution I could think of would require changing the non-blocking behaviour of BufferedReader.peek(), as described in bpo-13322. So I will leave that for later.

Adding ConnectionClosed.v3.patch; main changes:

• Removed the connection_reused flag to HTTPResponse
• ConnectionClosed raised even for the first request of a connection
• Added HTTPConnection.closed flag, which the user may check before a request to see if a fresh connection will be made, or an existing connection will be reused
• ConnectionClosed now subclasses both BadStatusLine and ConnectionError
• Fixed http.client.__all__ and added a somewhat automated test for it

BTW these patches kind of depend on bpo-5811 to confirm that BufferedReader.peek() will definitely return at least one byte unless at EOF.

If it would help the review process, I could simplify my patch by dropping the addition of the HTTPConnection.closed flag, so that it just adds the ConnectionClosed exception. Looking forwards, I’m wondering if it might be better to add something like a HTTPConnection.check_remote_closed() method instead of that flag anyway.

My apologies for the delay, but I've now reviewed the proposed patch. With a fresh outlook after taking a bit of time off, I'm not sure anymore that this is the best way of going about solving this problem. The main reason being that we now have two errors that effectively mean the same thing: The remote socket has encountered some error condition.

I understand that ConnectionClosed was added to maintain backwards compatibility with the BadStatusLine error, but I'm beginning to think that what really should be done is that backwards compatibility /should/ be broken as (in my mind) it's one of those cases where the backwards compatible solution may introduce just as many issues as it solves.

The root issue here (or at least what it has turned into) is that BadStatusLine is incorrectly raised when EOF is encountered when reading the status line. In light of that, I think that simply raising a ConnectionError in _read_status where line is None is the right way to fix this issue. Not only is it consistent with other cases where the remote socket is closed (i.e. when reading the response body), but it's removing the need for the addition of a potentially confusing exception.

I'm not 100% what the policy is around backwards introducing backwards incompatible changes is, but I really think that this is one of those few cases where it really should be broken.

Thanks for helping with this Demian. The idea of raising the same exception in all cases is new to me. Initially I was opposed, but it is starting to make sense. Let me consider it some more. Here are some cases that could trigger this exception:

1. EOF before receiving any status line. This is the most common case. Currently triggers BadStatusLine.
2. EOF in the middle of the status line. Triggers BadStatusLine, or is treated as an empty set of header fields.
3. EOF in the middle of a header line, or before the terminating blank line. Ignored, possibly with HTTPMessage.defects set.
4. EOF after receiving 100 Continue response, but before the final response. Currently triggers the same BadStatusLine.
5. ConnectionReset anywhere before the blank line terminating the header section.

In all those cases it should be okay to automatically retry an idempotent request. With non-idempotent requests, retrying in these cases seems about equally dangerous.

For contrast, some related cases that can still be handled differently:

6. Connection reset or broken pipe in the request() method, since the server can still send a response
7. Unexpected EOF or connection reset when reading the response body. Perhaps this could also be handled with a similar ConnectionError exception. Currently IncompleteRead is raised for EOF, at least in most cases. IncompleteRead has also been suggested as an alternative to BadStatusLine in the past.

Thanks for helping with this Demian.
No worries. This textual white boarding exercise has also been greatly
valuable in getting my own head wrapped around various low frequency
socket level errors that can be encountered when using the client. The
downside is that this issue is now quite likely very difficult for new
readers to follow :/

The idea of raising the same exception in all cases is new to me.
This initially puzzled me. I then re-read my response and realized that
I was thinking one thing and wrote another. The exception that I was
intending to suggest using here is ConnectionResetError, rather than the
ConnectionError base class. To elaborate a little further, as I
understand it, the /only/ case where the empty read happens is when the
remote connection has been closed but where the TCP still allows for EOS
to be read. In this case, the higher level implementation (in this case
the client) /knows/ that the empty line is signifying that the
connection has been closed by the remote host.

To be clear, a rough example of what I'm proposing is this:

diff -r e548ab4ce71d Lib/http/client.py
--- a/Lib/http/client.py        Mon Feb 09 19:49:00 2015 +0000
+++ b/Lib/http/client.py        Wed Feb 11 06:04:08 2015 -0800
@@ -210,7 +210,7 @@
         if not line:
             # Presumably, the server closed the connection before
             # sending a valid response.
-            raise BadStatusLine(line)
+            raise ConnectionResetError
         try:
             version, status, reason = line.split(None, 2)
         except ValueError:

Your example of the XML-RPC client would then use the alternative approach:

try:
return self.single_request(...)
except ConnectionError:
#retry request once if cached connection has gone cold
return self.single_request(...)

That all said, I'm also not tremendously opposed to the introduction of
the ConnectionClosed exception. I simply wanted to explore this thought
before the addition is made, although the comments in my patch review
would still hold true.

It is possible to break backward compatibility in a feature release if the break is fixing a bug. In this case I think it is in fact doing so, and that in fact in the majority of cases the change would either not break existing code or would even improve it (by making debugging easier). However, I have no way to prove that.

Often in the cases of compatibility breaks we will do a deprecation of the old behavior in a given release and make the change in the next release. I'm not convinced that is necessary (or even possible) here. It would be nice if we could get some data on what the actual impact would be on existing code. For example: how, if at all, would this affect the requests package? I *can* give one data point: in an application I wrote recently the affect would be zero, since every place in my application that I catch BadStatusLine I also catch ConnectionError.

I would want at least one other committer to sign off on a compatibility break before anything got committed.

Posting RemoteDisconnected.v4.patch with these changes:

• Renamed ConnectionClosed → RemoteDisconnected. Hopefully avoids confusion with shutting down the local end of a connection, or closing a socket’s file descriptor.

• Dropped the HTTPConnection.closed attribute

• Dropped special distinction of ECONNRESET at start versus in middle of response. It certainly makes the code and tests simpler again, and I realize that distinction is not the most important problem to solve right now, if ever. Also avoids relying on the poorly defined BufferedReader.peek() method.

I would like to retain the backwards compatibility with BadStatusLine if that is okay though.

Requests and “urllib3”: I’m not overly familiar with the internals of these packages (Requests uses “urllib3”). I cannot find any reference to BadStatusLine handling in “urllib3”, and I suspect it might just rely on detecting a dropped connection before sending a request; see <https://github.com/shazow/urllib3/blob/c8e7ea5/urllib3/connectionpool.py#L236\>. In my opinion this is a race condition, but it is helpful and works most of the time. So I suspect “urllib3” would not be affected by any changes made relating to BadStatusLine.

Left a few minor comments in Rietveld.

My only opposition to the RemoteDisconnected error is now we have two exceptions that effectively mean the same thing. It looks like asyncio.streams has similar behaviour: https://hg.python.org/cpython/file/041a27298cf3/Lib/asyncio/streams.py#l193. I think that if it's acceptable to break backwards compatibility here, we should.

Browsing through some Github repos, it seems like this change /could/ potentially impact a few smaller projects. I can confirm, however, that neither urllib3 nor requests are dependent on BadStatusLine.

I guess you saying RemoteDisconnected effectively means the same thing as ConnectionResetError. Would it help if it was derived from ConnectionResetError, instead of the ConnectionError base class? Or are you also worried about the multiple inheritance or clutter of yet another type of exception?

I’m not really familiar with the “asyncio” streams/protocols/transports/thingies, but it looks like the code you pointed to is actually called when writing, via drain(), fails. Maybe the equivalent code for when reading hits EOF is <https://hg.python.org/cpython/file/041a27298cf3/Lib/asyncio/streams.py#l239\>.

On Feb 19, 2015, at 8:08 PM, Martin Panter <report@bugs.python.org> wrote:
I guess you saying RemoteDisconnected effectively means the same thing as ConnectionResetError.

Exactly.

Would it help if it was derived from ConnectionResetError, instead of the ConnectionError base class? Or are you also worried about the multiple inheritance or clutter of yet another type of exception?

My concern is more about consistency of exceptions and exception handling when using the client. Thinking about it from a user’s standpoint, when I issue a request and the remote socket closes, I would hope to get consistent exceptions for all remote resets. If I’m handling the lowest level errors independently of one another rather than catch-all ConnectionError, I don’t want to do something like this:

except (RemoteDisconnected, ConnectionResetError)

I /should/ be able to simply use ConnectionResetError. Reading the docs, the only real reason for this exception at all is for backwards compatibility. If we have a case to break backwards compatibility here, then that eliminates the need for the new exception and potential (minor) confusion.

In this special case, the behaviour that we see at the client socket level indicates a remote reset, but it’s only artificially known immediately due to the empty read. In my mind, because the client /knows/ that this is an early indicator of a ConnectionResetError, that is exactly the exception that should be used.

Hope that makes sense.

Posting RemoteDisconnected.v5.patch:

• Rebased and fixed minor merge conflict
• Change RemoteDisconnected base class from ConnectionError to ConnectionResetError
• Minor tweaks to tests

It seems that having a separate RemoteDisconnected exception (as in this patch) has at least two benefits:

1. It would allow the user to distinguish between a true ConnectionResetError (due to TCP reset or whatever) from a clean TCP shutdown

2. Backwards compatibility with user code that only handles BadStatusLine

The only disadvantage seems to be the bloat of adding a new exception type. But if some other comitter agrees that merging them is better and dropping backwards compatibility is okay I am happy to adjust the patch to go along with that.

Pending review of the exceptions from another core dev, the patch looks good to me. Thanks for sticking with it :)

New changeset eba80326ba53 by R David Murray in branch 'default':
bpo-3566: Clean up handling of remote server disconnects.
https://hg.python.org/cpython/rev/eba80326ba53

Thanks, Martin and Demian. I tweaked the patch slightly before commit, so I've uploaded the diff. After thinking about it I decided that it does indeed make sense that the new exception subclass both ConnectionResetError and BadStatusLine, exactly because it *isn't* a pure ConnectionError, it is a synthetic one based on getting a '' response when we are expecting a status line. So I tweaked the language to not mention backward compatibility. I also tweaked the language of the docs, comments and error message to make it clear that the issue is that the server closed the connection (I understand why you changed it to 'shut down', but I think 'the server closed the connection' is both precise enough and more intuitive).

If you have any issues with the changes I made, let me know.

Your tweaks look fine. Thanks everyone for working through this one.