Martin Panter added the comment:
Your tweaks look fine. Thanks everyone for working through this one.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
R. David Murray added the comment:
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
Roundup Robot added the comment:
New changeset eba80326ba53 by R David Murray in branch 'default':
#3566: Clean up handling of remote server disconnects.
https://hg.python.org/cpython/rev/eba80326ba53
--
nosy: +python-dev
___
Python tracker
Changes by Demian Brecht demianbre...@gmail.com:
--
stage: patch review - commit review
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
___
Demian Brecht added the comment:
Pending review of the exceptions from another core dev, the patch looks good to
me. Thanks for sticking with it :)
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
Martin Panter added the comment:
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
Demian Brecht added the comment:
On Feb 19, 2015, at 8:08 PM, Martin Panter rep...@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
Martin Panter added the comment:
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
Demian Brecht added the comment:
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:
Martin Panter added the comment:
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
R. David Murray added the comment:
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
Demian Brecht added the comment:
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
Demian Brecht added the comment:
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
Martin Panter added the comment:
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
Martin Panter added the comment:
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
Martin Panter added the comment:
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
Demian Brecht added the comment:
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.
--
Martin Panter added the comment:
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
Martin Panter added the comment:
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
Demian Brecht added the comment:
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.
--
___
Python tracker rep...@bugs.python.org
Martin Panter added the comment:
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
Demian Brecht added the comment:
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
Demian Brecht added the comment:
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
Demian Brecht added the comment:
(Admittedly, I may also have been doing something entirely invalid in previous
experiments as well)
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
Martin Panter added the comment:
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
Martin Panter added the comment:
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;
Robert Collins added the comment:
FWIW, I agree with the analysis here, its standard HTTP behaviour in the real
world, and we should indeed handle it.
--
nosy: +rbcollins
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
Demian Brecht added the comment:
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
Demian Brecht added the comment:
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
Changes by Gregory P. Smith g...@krypto.org:
--
nosy: -gregory.p.smith
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
___
Martin Panter added the comment:
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
R. David Murray added the comment:
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
Demian Brecht added the comment:
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
Demian Brecht added the comment:
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
Martin Panter added the comment:
Spotted code in Python’s own library that maintains a persistent connection and
works around this issue:
Lib/xmlrpc/client.py:1142
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
Changes by R. David Murray rdmur...@bitdance.com:
--
nosy: +r.david.murray
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
___
Martin Panter added the comment:
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:
Martin Panter added the comment:
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
Demian Brecht added the comment:
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
Martin Panter added the comment:
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
Issue 8450, “False
Changes by Ian Cordasco graffatcolmin...@gmail.com:
--
nosy: +icordasc
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
___
Python-bugs-list
Changes by Demian Brecht demianbre...@gmail.com:
--
nosy: +demian.brecht
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
___
Alan Justino added the comment:
Seems to affect 2.7 too.
--
nosy: +alanjds
versions: +Python 2.7
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
Changes by Martin Panter vadmium...@gmail.com:
--
nosy: +vadmium
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
___
Python-bugs-list
Changes by Yuri Bochkarev baltazar...@gmail.com:
--
nosy: +Yuri.Bochkarev
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
___
Changes by Jesús Cea Avión j...@jcea.es:
--
nosy: +jcea
versions: +Python 3.3 -Python 2.7, Python 3.1, Python 3.2
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
Changes by Aron Griffis agrif...@n01se.net:
--
nosy: +agriffis
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
___
Python-bugs-list mailing
Changes by Alan Kennedy python-...@xhaus.com:
--
nosy: +amak
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
___
Python-bugs-list mailing
Changes by Senthil Kumaran orsent...@gmail.com:
--
assignee: - orsenthil
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
___
Gregory P. Smith g...@krypto.org added the comment:
unassigning, i don't had time to look at this one.
--
assignee: gregory.p.smith -
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
Changes by Senthil Kumaran orsent...@gmail.com:
--
nosy: +orsenthil
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
___
Python-bugs-list
Changes by Terry J. Reedy tjre...@udel.edu:
--
versions: +Python 2.7, Python 3.1, Python 3.2 -Python 2.5
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue3566
___
Gregory P. Smith g...@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
Changes by Gregory P. Smith [EMAIL PROTECTED]:
--
assignee: - gregory.p.smith
nosy: +gregory.p.smith
___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3566
___
C. Scott Ananian [EMAIL PROTECTED] added the comment:
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
#
Changes by Mark Hammond [EMAIL PROTECTED]:
--
nosy: +mhammond
___
Python tracker [EMAIL PROTECTED]
http://bugs.python.org/issue3566
___
___
Python-bugs-list mailing list
New submission from C. Scott Ananian [EMAIL PROTECTED]:
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
57 matches
Mail list logo
