httplib persistent connections violate MUST in RFC2616 sec 8.1.4. #47816
Comments
|
Although HTTP/1.1 says that servers SHOULD send a Connection: close httplib currently does not detect when the server closes its side of the 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 There appears to be code attempting to fix this problem in It would be best to extend the retry logic in request() to cover this |
|
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 There ought to be a better way to force the underlying OS to give you All of which is why the HTTP pipelining specs encourage you *not* to use |
|
btw, when using async io (poll, select, etc) I -think- your socket will I like your MSG_PEEK hack here. I'll figure out if this or something |
|
Not that we've removed the try one more time branch of the code, Jeremy On Sun, Aug 16, 2009 at 6:24 PM, Gregory P. Smith<report@bugs.python.org> wrote:
|
|
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, # To produce EPIPE: # ENOTCONN probably not possible with current Python, # To produce ECONNRESET: 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 Sometimes I get a BadStatusLine even for the second request: Platform: Linux-3.15.5-2-ARCH-x86_64-with-arch |
|
Spotted code in Python’s own library that maintains a persistent connection and works around this issue: |
|
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. |
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: |
|
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
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:
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") |
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?
This flow makes sense and is well accounted for with your proposed change. However, misbehaving cases such as:
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.
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:
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:
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.
+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. 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.
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.
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. |
+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:
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:
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:
|
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: That all said, I'm also not tremendously opposed to the introduction of |
|
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:
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\>. |
Exactly.
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:
It seems that having a separate RemoteDisconnected exception (as in this patch) has at least two benefits:
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': |
|
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. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: