Issue1346874
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2005-11-03 12:23 by cdwave, last changed 2022-04-11 14:56 by admin.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
issue1346874.patch | rharris, 2008-08-07 18:38 | adding 100-continue to httplib | review | |
issue1346874-273.patch | edevil, 2012-07-11 11:32 | Updated patch against 2.7.3 |
Pull Requests | |||
---|---|---|---|
URL | Status | Linked | Edit |
PR 14880 | open | tbartlett0, 2019-07-21 15:26 |
Messages (21) | |||
---|---|---|---|
msg60830 - (view) | Author: Mike Looijmans (cdwave) | Date: 2005-11-03 12:23 | |
I bumped into this code in httplib.py HTTPConnection, begin(): # read until we get a non-100 response while True: version, status, reason = self._read_status() if status != CONTINUE: break # skip the header from the 100 response while True: skip = self.fp.readline().strip() if not skip: break if self.debuglevel > 0: print "header:", skip This basically silently eats any 100-continue response that the server may send to us. This is not according to the spec - the client should WAIT for the 100-continue, and then post the data. Because of this code snippet, it is impossible for a client to wait for a 100-continue response, since it is silently eaten by this code. A correct implementation would be: - Check the outgoing headers for "Expect: 100-continue" - If that is present, set an "expectcontinue" flag. - If the client tries to send data to the connection, or if the data member was set in the request, wait for the server to send the 100 response BEFORE sending out any data at all, if the flag is set. - If the server fails to send it, the connection will eventually time out. I'd be happy to provide an implementation myself, as it doesn't seem hard to implement and would really help my project. |
|||
msg60831 - (view) | Author: Neal Norwitz (nnorwitz) * | Date: 2005-11-04 02:17 | |
Logged In: YES user_id=33168 It's much easier to get a patch integrated since there should be less work than a bug. I encourage you to work on a patch. Don't forget that a patch, must do many things: 1) fix the code, 2) fix (or add!) tests, 3) fix the documentation with an appropriate \versionchanged (or \versionadded) tag, and finally 4) update Misc/NEWS. The old behaviour should generally be backwards compatible by default when adding new functionality. If it's a bug, it may still be desirable to maintain backwards compatibility. I don't know enough about HTTP to provide any guidance here. |
|||
msg60832 - (view) | Author: John J Lee (jjlee) | Date: 2006-07-20 18:22 | |
Logged In: YES user_id=261020 I don't see any violation of RFC 2616 here. Which part of the spec., precisely, do you claim is violated? Still, implementing better 100 Continue handling would be useful, if done correctly, so go ahead and write that patch! |
|||
msg70848 - (view) | Author: Rick Harris (rharris) | Date: 2008-08-07 18:38 | |
I'm implemented the behavior described by Mike above with a patch to 2.6. The patch will raise an ExpectationFailed before sending the body if the server responds with a 417 (Expectation Failed). This patch should only modify behavior for requests that specify "Expect: 100-continue" in the request's headers. Note: The spec says that the client should send the body if the server does not respond within some reasonable period of time. This is not currently supported. Anyone have any thoughts on how that could be done? |
|||
msg134337 - (view) | Author: Carl Nobile (Carl.Nobile) | Date: 2011-04-24 17:33 | |
I have run into this same issue. It does violate RFC2616 in section 4.3 "All 1xx (informational), 204 (no content), and 304 (not modified) responses MUST NOT include a message-body. All other responses do include a message-body, although it MAY be of zero length." The embedded while loop is looking for entity data coming back from the server which will never be seen. In my tests the code dies with an exception. I don't see why anything is being done special for a 100 CONTINUE at all. My fix was to eliminate the code previously quoted and replace it with a single line of code so that it would now look like the code snippet below. def begin(self): if self.msg is not None: # we've already started reading the response return version, status, reason = self._read_status() self.status = status self.reason = reason.strip() Note on providing a patch as stated previously. Having this restriction on providing a patch is a large deterrent to people. I spent a lot of time myself finding the cause of the issues I was having. I don't really have the time to fix tests and documentation also. I understand the reason for asking, but it certainly is a discouragement to helping when bugs are found. |
|||
msg134403 - (view) | Author: Terry J. Reedy (terry.reedy) * | Date: 2011-04-25 19:02 | |
Carl, anyone is free to submit an incomplete patch, and people often do, but if people who are affected by an issue do not think it worth their time to complete it, or even move it along, there is no reason to expect people who are not affected by it to think so either. All of us volunteers are busy doing whatever it is we do. |
|||
msg165195 - (view) | Author: André Cruz (edevil) | Date: 2012-07-10 16:15 | |
Can anyone confirm what is missing for this patch to be committed? Is it just test and documentation changes or is something wrong with the code changes as well? |
|||
msg165196 - (view) | Author: Carl Nobile (Carl.Nobile) | Date: 2012-07-10 16:20 | |
I was told some time ago that it was documentation changes. And, if I remember correctly CONTINUE (100) was not ignored, it was actually broken. Data was being read from stdin when a CONTINUE was received and this should never happen based on RFC 2616, because there will never be any data to read. ~Carl On Tue, Jul 10, 2012 at 12:15 PM, André Cruz <report@bugs.python.org> wrote: > > André Cruz <andre@cabine.org> added the comment: > > Can anyone confirm what is missing for this patch to be committed? > > Is it just test and documentation changes or is something wrong with the > code changes as well? > > ---------- > nosy: +edevil > > _______________________________________ > Python tracker <report@bugs.python.org> > <http://bugs.python.org/issue1346874> > _______________________________________ > |
|||
msg165205 - (view) | Author: André Cruz (edevil) | Date: 2012-07-10 21:32 | |
As far as I can see, the patch does add some documentation changes. What exactly is missing? As for the bug, if I understood correctly, what you are saying is that when "ignore_continue" is True, and the server sends a 100 Continue response, the code will continue to try to read data from the server even though none is expected, is that right? |
|||
msg165207 - (view) | Author: Carl Nobile (Carl.Nobile) | Date: 2012-07-10 22:14 | |
Yes, exactly. I was not the one who posted the original bug report, but I found it when I ran into the same problem. I was not exactly sure if the original poster had the same issues as I had. I do know that my fix to the code eliminated some code making the code a bit simpler. I could try to dig up my fix and send it to you if you want. ~Carl On Tue, Jul 10, 2012 at 5:32 PM, André Cruz <report@bugs.python.org> wrote: > > André Cruz <andre@cabine.org> added the comment: > > As far as I can see, the patch does add some documentation changes. What > exactly is missing? > > As for the bug, if I understood correctly, what you are saying is that > when "ignore_continue" is True, and the server sends a 100 Continue > response, the code will continue to try to read data from the server even > though none is expected, is that right? > > ---------- > > _______________________________________ > Python tracker <report@bugs.python.org> > <http://bugs.python.org/issue1346874> > _______________________________________ > |
|||
msg165209 - (view) | Author: André Cruz (edevil) | Date: 2012-07-10 23:09 | |
Carl: that would be great. Do you use it regularly? Any other problems? Python devs: can anyone confirm me what still needs to be done so that this patch can be considered for merging into trunk? Thanks. |
|||
msg165211 - (view) | Author: Senthil Kumaran (orsenthil) * | Date: 2012-07-10 23:46 | |
The patch seems good. I apologize that this has been sitting stale for a long time. Since this is a new feature, I am not sure if putting to 3.3 might be a good idea. This is a feature for httplib, so it may not make it to 2.7.x, but can make it to 3.4. |
|||
msg165220 - (view) | Author: Carl Nobile (Carl.Nobile) | Date: 2012-07-11 02:08 | |
André, As I said I'm not sure if I am fixing the same thing that this bug was originally posted for. However, after looking at my code I realized that I just did a quick work around for my situation and it shouldn't be put into any python release. This issue is that after a client receives a CONTINUE the client needs to send the balance of the data. My issue is that the client wouldn't always do that and the server would hang. So there seems to be a bug with regard to a reasonable timeout. When I write RESTful web services I often need to implement CONTINUE, but this status is rarely used, so I guess I don't use it too regularly. ~Carl On Tue, Jul 10, 2012 at 7:46 PM, Senthil Kumaran <report@bugs.python.org>wrote: > > Senthil Kumaran <senthil@uthcode.com> added the comment: > > The patch seems good. I apologize that this has been sitting stale for a > long time. Since this is a new feature, I am not sure if putting to 3.3 > might be a good idea. This is a feature for httplib, so it may not make it > to 2.7.x, but can make it to 3.4. > > ---------- > > _______________________________________ > Python tracker <report@bugs.python.org> > <http://bugs.python.org/issue1346874> > _______________________________________ > |
|||
msg165252 - (view) | Author: André Cruz (edevil) | Date: 2012-07-11 11:32 | |
Attached is an updated patch against 2.7.3. It solves a bug in the tests and implements Carl's suggestion. The new tests pass and it updates the documentation as well. As for inclusion in 2.7, as this is in fact solving a bug, I would vote for it. Right now, if the client sends an "Expect" header, httplib will send the body of the request along, which is against the RFC. It is backwards-compatible. |
|||
msg241188 - (view) | Author: Martin Panter (martin.panter) * | Date: 2015-04-16 01:42 | |
This seems more like a new feature than a bug fix to me. Currently the behaviour is the same whether “Expect: 100-continue” is requested or not. According to <https://tools.ietf.org/html/rfc7231#page-35>, the client does not have to wait any specific length of time for the “100 Continue” response, so sending the body straight away is okay. In current releases (2.7, 3.4), maybe it would be good enough to document that the body is always sent without waiting, even if “Expect: 100-continue” is requested. Considering this as a new feature, the new exceptions don’t seem very flexible to me. What is the use case, or what would your code look like? If I wanted to use the expectation feature of HTTP, I think I would want to know the reason why “100 Continue” was not received, and the blanket ContinueExpected exception is not going to be very helpful with that. Maybe I am being redirected somewhere else first, or maybe I have to log in with some kind of authentication, etc, but there is no way to get the relevant Location, WWW-Authenticate, etc fields. Some more comments specifically about issue1346874-273.patch: * The documentation should say what the behaviour is when “Expect: 100-continue” is requested. Point out that some sort of response is expected from the server before sending the body, and if the server does not send such a response the request will deadlock or time out. * __all__ would need updating with the new exceptions * “100 Continue” responses should still be read even when the expectation field is not in the request (and presumably, second and subsequent “100 Continue” responses should also be skipped even when the expectation is enabled). If there is not a test for this, one should probably be added. |
|||
msg262236 - (view) | Author: Patrick J McNerthney (IcicleSpider) | Date: 2016-03-23 03:24 | |
I believe this is actually a bug, because it causes the posting of messages whose length is greater then the Apache HTTPD SSLRenegBufferSize setting to fail, whereas other http clients that first wait to receive the 100 response before sending the body do work. The situation I am encountering is an Apache server that uses client certificates to authorize calls to it's rest apis, whereas the rest of the same site do not require it. See the documentation about the SSLRegenBufferSize here: https://httpd.apache.org/docs/trunk/mod/mod_ssl.html#sslrenegbuffersize By sending just the headers first with the "Expect: 100-continue" header and then waiting for the "100" response, Apache only has to buffer the headers. But the way HTTPConnection currently works, the headers and the body will be sent immediately, causing the SSLRenegBuffer to be exceed, causing the request to fail. Also, I do not think the submitted patch is the best fix for this. The HTTPConnection.putheader method should be the place that checks to see if there is an Expect header sent. |
|||
msg262801 - (view) | Author: Martin Panter (martin.panter) * | Date: 2016-04-02 22:31 | |
Patrick, can you elaborate on the problem with Apache and renegotiation? How does the 100-continue request, or anticipating the 100 response, help? Perhaps Issue 25919 is relevant in this case, where we want the client to receive an error response before the request data is completely sent. |
|||
msg262818 - (view) | Author: Patrick J McNerthney (IcicleSpider) | Date: 2016-04-03 08:19 | |
Martin, My understanding of the intention of the "Expect: 100-continue" header would address being able to receive an error response that is determined solely from the http request headers sent. So I do think that that would be the proper way to address that other issue, and without having to worry about sending and receiving data at once. However, what I am running into is a different issue caused by the same root cause of not properly handling "Expect: 100-continue" header in the client. In my case, what is occurring is the URL being called is configured to require an SSL client certificate. Apache is only able to determine that the client certificate is required after the http request headers are sent. Once Apache has the headers and determines that the client certificate is required, the ssl connection is renegotiated. While performing this renegotiation, Apache holds the request data in progress in a "SSLRenegBuffer". If the data sent is larger then the SSLRenegBufferSize configuration, then Apache will fail the request as being to large. With the current HttpsConnection, the sequence of events leading to the failure are: 1. HttpsConnection sends all the headers, including the Expect: 100-continue header, and sends all the 128K of data as fast as the socket allows. 2. Apache receives the request headers and looks at the request URL and determines that a client certificate is required, precipitating a ssl renegotiation. 3. Apache attempts to buffer the data received so far so that the ssl renegotiation can occur. However, the body of the request has already been sent and it is larger then SSLRenegBufferSize. 4. Apache fails the request as being to large. 5. HttpsConnection gets the error response about too large of a request. I have created my own fork of httplib.py that does proper Expect: 100-continue handling, requesting in the following successful steps: 1. HttpsConnection sends all headers, including the Expect: 100-continue header. The Expect: 100-continue header was detected, so HttpsConnection does not send the body yet. 2. HttpsConnection waits for a response. 3. Apache httpd receives the headers and detects that the request url requires a client certificate. 4. The ssl connection is renegotiated using the client certificate. 5. Apache sends the response status of 100. This is the only header sent, in addition to a blank line. 6. HttpsConnection receives the response status 100, which means all is good to go. 7. HttpsConnection sends the body of the request. 8. Apache gets the request body, which now can be very, very large, because it does not have to be held in the SSLRenegBuffer. 9. Apache processes request and returns the response. 10. HttpsConnection receives the response and processes. Note that if Apache detected some error detected with the headers, then the sequence would go like so: 1. HttpsConnection sends all headers, including the Expect: 100-continue header. The Expect: 100-continue header was detected, so HttpsConnection does not send the body yet. 2. HttpsConnection waits for a response. 3. Apache httpd receives the headers and detects some kind of error. Maybe the URL is not handled at all. 4. Apache sends the error response. 5. HttpsConnection receives the error response and uses that as the final response of the request. The body of the request is never sent at all. One unexpected side effect of using the Expect: 100-continue header I found was that requests smaller then the SSLRenegBufferSize were handled by Apache almost 3 times faster then without the Expect: 100-continue header. I assume this is because Apache does have to mess around with all the extra overhead of staging the request in the SSLRenegBuffer, but can instead route the body of the request directly to the request handler. |
|||
msg262827 - (view) | Author: Martin Panter (martin.panter) * | Date: 2016-04-03 12:04 | |
Thankyou, I think I understand your situation better now (though I don’t understand why Apache doesn’t renegotiate while the request body is being sent). I would still argue that this is a new feature to be added rather than a bug though, and should only go into Python 3.6+. The original HTTP 1.1 specification (RFC 2068) did not even mention an “Expect: 100-continue” header field, although it does mention the “100 Continue” response. And any change will probably need new APIs and documentation. Rather than parsing the headers for “100-continue”, I wonder if it would be cleaner adding an API flag to add the header. Or perhaps a new method that explicitly waits for the 100 response. |
|||
msg262832 - (view) | Author: Patrick J McNerthney (IcicleSpider) | Date: 2016-04-03 20:54 | |
"(though I don’t understand why Apache doesn’t renegotiate while the request body is being sent)" Apache does attempt to do this, but HttpsConnection is immediately sending the body of the request as fast as the socket will allow, which fills up the SSLRenegBuffer before the renegotiation can occur. "Or perhaps a new method that explicitly waits for the 100 response." That is likely a good idea. My httplib.py fork did change the behavior of the endheaders method to return a response object if there was an error returned in response to the "Expect: 100-continue". |
|||
msg348250 - (view) | Author: Tim B (tbartlett0) * | Date: 2019-07-21 15:36 | |
I've created a PR to potentially implement this in 3.9. Please take a look and review/test, if this issue is still relevant to you. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:56:13 | admin | set | github: 42550 |
2020-11-04 17:34:38 | Mattias Amnefelt | set | nosy:
+ Mattias Amnefelt |
2019-07-21 16:05:29 | terry.reedy | set | nosy:
- terry.reedy versions: - Python 3.1, Python 2.7, Python 3.2 |
2019-07-21 15:36:57 | tbartlett0 | set | versions: + Python 3.9 |
2019-07-21 15:36:27 | tbartlett0 | set | nosy:
+ tbartlett0 messages: + msg348250 |
2019-07-21 15:26:04 | tbartlett0 | set | pull_requests: + pull_request14665 |
2017-09-02 19:17:13 | gms | set | nosy:
+ gms |
2016-04-03 20:54:35 | IcicleSpider | set | messages: + msg262832 |
2016-04-03 12:04:18 | martin.panter | set | messages: + msg262827 |
2016-04-03 08:19:31 | IcicleSpider | set | messages: + msg262818 |
2016-04-02 22:31:56 | martin.panter | set | messages: + msg262801 |
2016-03-23 03:24:29 | IcicleSpider | set | nosy:
+ IcicleSpider messages: + msg262236 |
2015-04-16 01:42:12 | martin.panter | set | nosy:
+ martin.panter messages: + msg241188 |
2012-07-11 11:32:51 | edevil | set | files:
+ issue1346874-273.patch messages: + msg165252 |
2012-07-11 02:08:52 | Carl.Nobile | set | messages: + msg165220 |
2012-07-10 23:46:06 | orsenthil | set | messages: + msg165211 |
2012-07-10 23:09:54 | edevil | set | messages: + msg165209 |
2012-07-10 22:14:50 | Carl.Nobile | set | messages: + msg165207 |
2012-07-10 21:32:25 | edevil | set | messages: + msg165205 |
2012-07-10 16:20:49 | Carl.Nobile | set | messages: + msg165196 |
2012-07-10 16:15:06 | edevil | set | nosy:
+ edevil messages: + msg165195 |
2011-08-07 01:46:40 | nikratio | set | nosy:
+ nikratio |
2011-04-25 19:02:43 | terry.reedy | set | nosy:
+ terry.reedy messages: + msg134403 |
2011-04-24 17:33:51 | Carl.Nobile | set | nosy:
+ Carl.Nobile messages: + msg134337 |
2010-08-22 08:52:21 | eric.araujo | set | assignee: orsenthil nosy: + orsenthil |
2010-08-22 01:23:13 | BreamoreBoy | set | stage: patch review type: behavior versions: + Python 3.1, Python 2.7, Python 3.2, - Python 2.6 |
2008-08-07 18:38:33 | rharris | set | files:
+ issue1346874.patch nosy: + rharris messages: + msg70848 keywords: + patch versions: + Python 2.6, - Python 2.4 |
2005-11-03 12:23:56 | cdwave | create |