#3566: httplib persistent connections violate MUST in RFC2616 sec 8.1.4.
|
Created:
4 years ago by cscott Modified:
3 years, 10 months ago Reviewers:
demianbrecht, rdmurray, vadmium+py CC:
jeremy_alum.mit.edu, mhammond, jcea, orsenthil, python-dev_xhaus.com, rbcollins, cscott_cscott.net, r.david.murray, alan.justino_yahoo.com.br, agriffis_n01se.net, devnull_psf.upfronthosting.co.za, berkerpeksag, Martin Panter, icordasc, demian, baltazar.bz_gmail.com Visibility:
Public. |
Patch Set 1 #
Total comments: 6
Patch Set 2 #
Total comments: 10
Patch Set 3 #
Total comments: 4
Patch Set 4 #
Total comments: 6
Patch Set 5 #Patch Set 6 #
Created: 3 years, 10 months ago
MessagesTotal messages: 9
http://bugs.python.org/review/3566/diff/13714/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/3566/diff/13714/Lib/http/client.py#newcode243 Lib/http/client.py:243: if self._connection_reused: I don't think that this sits quite right with me from an architectural perspective. A response, as a self-contained entity, should not have to know about the state of previous responses. If anything, the higher level code using the response should be, whether it's HTTPConnection or the user's code if using HTTPResponse directly for some reason. In addition to that, is this /actually/ needed? Based on the research and testing that I've done, the only way that a client will /ever/ get an empty read from the receive buffer is on a disconnect. The server can't send and empty response as it results in a no-op, and the client won't be notified of an incoming message by whatever select() method it's using until EOS due to the disconnect. In light of that (unless I'm missing something), I'd be entirely happy for the following block to replace the BadStatusLine exception here https://hg.python.org/cpython/file/a858cde113f2/Lib/http/client.py#l210. I'm happy that you handle the ConnectionError here too. I ran into a ConnectionResetError and was going to put together a patch for it. No need to now :) http://bugs.python.org/review/3566/diff/13714/Lib/http/client.py#newcode1131 Lib/http/client.py:1131: self.close() Nice catch here too to close the socket and reset the state machine. http://bugs.python.org/review/3566/diff/13714/Lib/test/test_httplib.py File Lib/test/test_httplib.py (right): http://bugs.python.org/review/3566/diff/13714/Lib/test/test_httplib.py#newcode46 Lib/test/test_httplib.py:46: def readable(self): Each method should have a black line between itself and the previous function block. http://bugs.python.org/review/3566/diff/13714/Lib/test/test_httplib.py#newcod... Lib/test/test_httplib.py:1021: ('1.0', 'keep-ALIVE', True), Can these be standard casing here (i.e. "Keep-Alive")? I'm assuming that the intention here is to test the persistence behaviour and not the case handling. If that's the case, this will just confuse the person reading the test code (as it did me).
Sign in to reply to this message.
This is an API review based on the docs, I haven't looked at the code patch. http://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst File Doc/library/http.client.rst (right): http://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst#new... Doc/library/http.client.rst:96: .. class:: HTTPResponse(sock, debuglevel=0, method=None, url=None, connection_reused=False) You have provided no documentation for this attribute, and I don't understand its purpose. (I agree with Damien that it doesn't seem like a good idea for the response to need to know anything about the connection state.) http://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst#new... Doc/library/http.client.rst:184: If ConnectionError could be raised here, and you are trying to retain backward compatibility, shouldn't it subclass both ConnectionError and BadStatusLine?) http://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst#new... Doc/library/http.client.rst:249: a new request is sent. Why should it be BadStatusLine if it is the first request? If the connection is closed, I would expect to get a ConnectionClosed error in that case as well. http://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst#new... Doc/library/http.client.rst:258: after a persistent connection is dropped. Shouldn't this note be under 'close' or where persistent connections are discussed (assuming there is such a place)?
Sign in to reply to this message.
http://bugs.python.org/review/3566/diff/13714/Lib/test/test_httplib.py File Lib/test/test_httplib.py (right): http://bugs.python.org/review/3566/diff/13714/Lib/test/test_httplib.py#newcod... Lib/test/test_httplib.py:1021: ('1.0', 'keep-ALIVE', True), On 2015/01/25 07:55:27, demian wrote: > Can these be standard casing here (i.e. "Keep-Alive")? I'm assuming that the > intention here is to test the persistence behaviour and not the case handling. > If that's the case, this will just confuse the person reading the test code (as > it did me). What about if I duplicated these lines with the normal "keep-alive" casing, and then added a comment saying “Test case insensitivity” to the non-standard lines? Looking at the code, the case insensitive comparison is special to the _check_close() method which checks these field values, so it would be good to cover that in the test suite somewhere. http://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst File Doc/library/http.client.rst (right): http://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst#new... Doc/library/http.client.rst:184: On 2015/01/25 22:21:24, r.david.murray wrote: > If ConnectionError could be raised here, and you are trying to retain backward > compatibility, shouldn't it subclass both ConnectionError and BadStatusLine?) I decided not to bother because the more common case seems to be BadStatusLine. But it wouldn’t really hurt inheriting both, so I will reconsider when I revise the patch. http://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst#new... Doc/library/http.client.rst:258: after a persistent connection is dropped. On 2015/01/25 22:21:24, r.david.murray wrote: > Shouldn't this note be under 'close' or where persistent connections are > discussed (assuming there is such a place)? Perhaps, if such a place existed. In the module doc string (via pydoc) there is a state machine. Perhaps it could be updated and integrated into the reference documentation, and then the close behaviour might fit in well with that.
Sign in to reply to this message.
http://bugs.python.org/review/3566/diff/13714/Lib/test/test_httplib.py File Lib/test/test_httplib.py (right): http://bugs.python.org/review/3566/diff/13714/Lib/test/test_httplib.py#newcod... Lib/test/test_httplib.py:1021: ('1.0', 'keep-ALIVE', True), On 2015/01/26 02:11:37, vadmium wrote: > What about if I duplicated these lines with the normal "keep-alive" casing, and then added a comment saying “Test case insensitivity” to the non-standard lines? Sounds good to me, that would definitely eliminate any confusion. I don't think it makes sense to split up the test methods based on function (which is what I'd generally be in favour of) if the logic is specific to _check_close. http://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst File Doc/library/http.client.rst (right): http://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst#new... Doc/library/http.client.rst:249: a new request is sent. On 2015/01/25 22:21:24, r.david.murray wrote: > Why should it be BadStatusLine if it is the first request? If the connection is > closed, I would expect to get a ConnectionClosed error in that case as well. +1 http://bugs.python.org/review/3566/diff/13719/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/3566/diff/13719/Lib/http/client.py#newcode243 Lib/http/client.py:243: if self._connection_reused: It seems that my first comment didn't persist for some reason, so re-adding it for posterity: I don't think it makes sense for the response object to be aware of the connection state. I'd be happy with the following logic entirely replacing the BadStatusLine handling here: https://hg.python.org/cpython/file/db7ec64aac39/Lib/http/client.py#l210. I'm wondering if the peek() line is actually needed. To the user calling this, do you think there's a need to distinguish between a connection that reset after a few bytes and one that resets without responding with anything? Both cases result in a dropped connection and I can't think of a case where the distinction would make any difference to me as a user. I really don't have a strong opinion either way, just asking the question.
Sign in to reply to this message.
https://bugs.python.org/review/3566/diff/13719/Lib/http/client.py File Lib/http/client.py (right): https://bugs.python.org/review/3566/diff/13719/Lib/http/client.py#newcode243 Lib/http/client.py:243: if self._connection_reused: On 2015/01/26 16:52:32, demian wrote: > I'm wondering if the peek() line is actually needed. To the user calling this, > do you think there's a need to distinguish between a connection that reset after > a few bytes and one that resets without responding with anything? If the connection resets after receiving part of the status line, then the server has probably seen the request, and I don’t think any ConnectionClosed handling like automatic retrying should apply. I think the behaviour in this case should be closer to a connection reset in the headers after the status line.
Sign in to reply to this message.
http://bugs.python.org/review/3566/diff/13795/Doc/library/http.client.rst File Doc/library/http.client.rst (right): http://bugs.python.org/review/3566/diff/13795/Doc/library/http.client.rst#new... Doc/library/http.client.rst:174: A subclass of :exc:`ConnectionError`. Raised by Maybe move the "This is also a subclass..." from the versionadded block up here? It's a little strange when reading through this that the parent classes are segregated. Although I understand why, others not familiar with the history of this issue would likely be puzzled. http://bugs.python.org/review/3566/diff/13795/Lib/http/client.py File Lib/http/client.py (right): http://bugs.python.org/review/3566/diff/13795/Lib/http/client.py#newcode245 Lib/http/client.py:245: try: I think I'd prefer to see this logic inline with where these kinds of errors are currently raised. For example, the case where ConnectionClosed is raised due to no response should likely be raised in _read_status where line is None and BadStatusLine is currently raised, which is made redundant with this new change. http://bugs.python.org/review/3566/diff/13795/Lib/http/client.py#newcode810 Lib/http/client.py:810: def closed(self): Given the potentially volatile nature of the remote socket, I'm not sure how I feel about this. I would think that to those less familiar with the socket level side of things, it might lead to confusion. For example, consider the following case: if not con.closed: con.request('GET', '/') r = con.getresponse() # which can potentially raise ConnectionClosed I think that due to various error conditions that (especially) a remote socket can encounter, this property should just be removed as, to me, it's really indicating that the HTTPConnection at /both/ ends is closed, which is much more complex than simply testing whether or not the socket is None.
Sign in to reply to this message.
https://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst File Doc/library/http.client.rst (right): https://bugs.python.org/review/3566/diff/13719/Doc/library/http.client.rst#ne... Doc/library/http.client.rst:258: after a persistent connection is dropped. On 2015/01/25 22:21:24, r.david.murray wrote: > Shouldn't this note be under 'close' or where persistent connections are > discussed (assuming there is such a place)? I will drop the reference to close() in my next patch. Hopefully it will seem more appropriate that way. https://bugs.python.org/review/3566/diff/13795/Lib/http/client.py File Lib/http/client.py (right): https://bugs.python.org/review/3566/diff/13795/Lib/http/client.py#newcode810 Lib/http/client.py:810: def closed(self): On 2015/02/10 16:56:17, demian wrote: > I think that due to various error conditions that (especially) a remote socket > can encounter, this property should just be removed as, to me, it's really > indicating that the HTTPConnection at /both/ ends is closed, which is much more > complex than simply testing whether or not the socket is None. Yes I sort of agree. Next patch iteration I plan to remove this feature so we can focus on just the ConnectionClosed exception.
Sign in to reply to this message.
http://bugs.python.org/review/3566/diff/13970/Doc/library/http.client.rst File Doc/library/http.client.rst (right): http://bugs.python.org/review/3566/diff/13970/Doc/library/http.client.rst#new... Doc/library/http.client.rst:288: called automatically when making a request if the client does not already This is only true when auto_open == True. http://bugs.python.org/review/3566/diff/13970/Lib/test/test_httplib.py File Lib/test/test_httplib.py (right): http://bugs.python.org/review/3566/diff/13970/Lib/test/test_httplib.py#newcod... Lib/test/test_httplib.py:1016: ('1.0', None, False), Why not default the header value to empty string here rather than None so the conversion doesn't have to be done further down? http://bugs.python.org/review/3566/diff/13970/Lib/test/test_httplib.py#newcod... Lib/test/test_httplib.py:1046: if reuse: This could be simplified to self.assertEqual(conn.connections, 1 if reuse else 2) http://bugs.python.org/review/3566/diff/13970/Lib/test/test_httplib.py#newcod... Lib/test/test_httplib.py:1074: # HTTPConnection.connect() should be automatically invoked It might be worthwhile testing NotConnected is raised if conn.auto_connect is set to False. I don't think it's being tested anywhere else at the moment.
Sign in to reply to this message.
https://bugs.python.org/review/3566/diff/13970/Doc/library/http.client.rst File Doc/library/http.client.rst (right): https://bugs.python.org/review/3566/diff/13970/Doc/library/http.client.rst#ne... Doc/library/http.client.rst:288: called automatically when making a request if the client does not already On 2015/02/20 02:26:00, demian wrote: > This is only true when auto_open == True. But auto_open is undocumented, and only set to 1 (a.k.a. True) internally, so it does not seem appropriate to mention this. Maybe I will say “By default, this is called automatically . . .” https://bugs.python.org/review/3566/diff/13970/Lib/test/test_httplib.py File Lib/test/test_httplib.py (right): https://bugs.python.org/review/3566/diff/13970/Lib/test/test_httplib.py#newco... Lib/test/test_httplib.py:1074: # HTTPConnection.connect() should be automatically invoked On 2015/02/20 02:26:00, demian wrote: > It might be worthwhile testing NotConnected is raised if conn.auto_connect is > set to False. I don't think it's being tested anywhere else at the moment. Since this feature is undocumented, rarely used, and I don’t know the details, I would prefer to leave that for a separate patch.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
