-
-
Notifications
You must be signed in to change notification settings - Fork 34.7k
gh-42550: Add 'Expect: 100-Continue' support to httplib #133276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
3c37a5e
532026d
7146d5b
d902673
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -74,6 +74,7 @@ | |||||
| import http | ||||||
| import io | ||||||
| import re | ||||||
| import select | ||||||
| import socket | ||||||
| import sys | ||||||
| import collections.abc | ||||||
|
|
@@ -327,15 +328,16 @@ def _read_status(self): | |||||
| raise BadStatusLine(line) | ||||||
| return version, status, reason | ||||||
|
|
||||||
| def begin(self, *, _max_headers=None): | ||||||
| def begin(self, ignore_100_continue=True, _max_headers=None): | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the
Suggested change
|
||||||
| if self.headers is not None: | ||||||
| # we've already started reading the response | ||||||
| return | ||||||
|
|
||||||
| # read until we get a non-100 response | ||||||
| # (unless caller has requested return of 100 responses) | ||||||
| while True: | ||||||
| version, status, reason = self._read_status() | ||||||
| if status != CONTINUE: | ||||||
| if not (ignore_100_continue and status == CONTINUE): | ||||||
| break | ||||||
| # skip the header from the 100 response | ||||||
| skipped_headers = _read_headers(self.fp, _max_headers) | ||||||
|
|
@@ -888,13 +890,16 @@ def _get_content_length(body, method): | |||||
| return None | ||||||
|
|
||||||
| def __init__(self, host, port=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT, | ||||||
| source_address=None, blocksize=8192, *, max_response_headers=None): | ||||||
| source_address=None, blocksize=8192, continue_timeout=2.5, *, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. continue_timeout should be a keyword only argument.... but is the constructur really where we want it rather than on the request() method where headers are specified? That'd also avoid the need to plumb this through From an API design perspective, on the HTTPConnection.request method which takes a headers={} dict as well as some flags such as potentially this... If someone wants to use this feature, it is ergonomically nicer if we had a flag on the request_api that explicitly adds the correct also, assert that continue_timeout > 0.0. |
||||||
| max_response_headers=None): | ||||||
| self.timeout = timeout | ||||||
| self.continue_timeout = continue_timeout | ||||||
| self.source_address = source_address | ||||||
| self.blocksize = blocksize | ||||||
| self.sock = None | ||||||
| self._buffer = [] | ||||||
| self.__response = None | ||||||
| self.__pending_response = None | ||||||
| self.__state = _CS_IDLE | ||||||
| self._method = None | ||||||
| self._tunnel_host = None | ||||||
|
|
@@ -907,9 +912,10 @@ def __init__(self, host, port=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT, | |||||
|
|
||||||
| self._validate_host(self.host) | ||||||
|
|
||||||
| # This is stored as an instance variable to allow unit | ||||||
| # tests to replace it with a suitable mockup | ||||||
| # These are stored as instance variables to allow unit | ||||||
| # tests to replace them with a suitable mockup | ||||||
| self._create_connection = socket.create_connection | ||||||
| self._select = select.select | ||||||
|
|
||||||
| def set_tunnel(self, host, port=None, headers=None): | ||||||
| """Set up host and port for HTTP CONNECT tunnelling. | ||||||
|
|
@@ -1045,6 +1051,10 @@ def close(self): | |||||
| self.sock = None | ||||||
| sock.close() # close it manually... there may be other refs | ||||||
| finally: | ||||||
| pending_response = self.__pending_response | ||||||
| if pending_response: | ||||||
| self.__pending_response = None | ||||||
| pending_response.close() | ||||||
| response = self.__response | ||||||
| if response: | ||||||
| self.__response = None | ||||||
|
|
@@ -1105,7 +1115,8 @@ def _read_readable(self, readable): | |||||
| datablock = datablock.encode("iso-8859-1") | ||||||
| yield datablock | ||||||
|
|
||||||
| def _send_output(self, message_body=None, encode_chunked=False): | ||||||
| def _send_output(self, message_body=None, encode_chunked=False, | ||||||
| expect_continue=False): | ||||||
| """Send the currently buffered request and clear the buffer. | ||||||
|
|
||||||
| Appends an extra \\r\\n to the buffer. | ||||||
|
|
@@ -1117,6 +1128,23 @@ def _send_output(self, message_body=None, encode_chunked=False): | |||||
| self.send(msg) | ||||||
|
|
||||||
| if message_body is not None: | ||||||
| if expect_continue and not self.__response: | ||||||
| read_ready, _, _ = self._select([self.sock], [], [], | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. select won't work properly on a HTTPSConnection. There's a .pending method on SSLSocket that could be used in this scenario if read_ready. Investigate what is plausible and add some tests covering the HTTPSConnection test case. |
||||||
| self.continue_timeout) | ||||||
| if read_ready: | ||||||
| if self.debuglevel > 0: | ||||||
| response = self.response_class(self.sock, | ||||||
| self.debuglevel, | ||||||
| method=self._method) | ||||||
| else: | ||||||
| response = self.response_class(self.sock, | ||||||
| method=self._method) | ||||||
| response.begin(ignore_100_continue=False) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. _max_headers= should also be passed here. |
||||||
| if response.code != CONTINUE: | ||||||
| # Break without sending the body | ||||||
| self.__pending_response = response | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wherever self.__response is cleared (= None) such as putrequest, self.__pending_response needs to cleared as well. tests should cover that this happens by running through a scenario that'd lead to this such as maybe a non-100 early response that gets ignored before another request is sent on the same connection? |
||||||
| return | ||||||
| response.close() | ||||||
|
|
||||||
| # create a consistent interface to message_body | ||||||
| if hasattr(message_body, 'read'): | ||||||
|
|
@@ -1343,7 +1371,8 @@ def putheader(self, header, *values): | |||||
| header = header + b': ' + value | ||||||
| self._output(header) | ||||||
|
|
||||||
| def endheaders(self, message_body=None, *, encode_chunked=False): | ||||||
| def endheaders(self, message_body=None, *, encode_chunked=False, | ||||||
| expect_continue=False): | ||||||
| """Indicate that the last header line has been sent to the server. | ||||||
|
|
||||||
| This method sends the request to the server. The optional message_body | ||||||
|
|
@@ -1354,7 +1383,8 @@ def endheaders(self, message_body=None, *, encode_chunked=False): | |||||
| self.__state = _CS_REQ_SENT | ||||||
| else: | ||||||
| raise CannotSendHeader() | ||||||
| self._send_output(message_body, encode_chunked=encode_chunked) | ||||||
| self._send_output(message_body, encode_chunked=encode_chunked, | ||||||
| expect_continue=expect_continue) | ||||||
|
|
||||||
| def request(self, method, url, body=None, headers={}, *, | ||||||
| encode_chunked=False): | ||||||
|
|
@@ -1399,19 +1429,33 @@ def _send_request(self, method, url, body, headers, encode_chunked): | |||||
| else: | ||||||
| encode_chunked = False | ||||||
|
|
||||||
| # If the Expect: 100-continue header is set, we will try to honor it | ||||||
| # (if possible). We can only do so if 1) the request has a body, and | ||||||
| # 2) there is no current incomplete response (since we need to read | ||||||
| # the response stream to check if the code is 100 or not) | ||||||
| expect_continue = ( | ||||||
| body and not self.__response | ||||||
| and 'expect' in header_names | ||||||
| and '100-continue' in {v.lower() for k, v in headers.items() | ||||||
| if k.lower() == 'expect'} | ||||||
| ) | ||||||
|
|
||||||
| for hdr, value in headers.items(): | ||||||
| self.putheader(hdr, value) | ||||||
| if isinstance(body, str): | ||||||
| # RFC 2616 Section 3.7.1 says that text default has a | ||||||
| # default charset of iso-8859-1. | ||||||
| body = _encode(body, 'body') | ||||||
| self.endheaders(body, encode_chunked=encode_chunked) | ||||||
| self.endheaders(body, encode_chunked=encode_chunked, | ||||||
| expect_continue=expect_continue) | ||||||
|
|
||||||
| def getresponse(self): | ||||||
| def getresponse(self, ignore_100_continue=True): | ||||||
| """Get the response from the server. | ||||||
|
|
||||||
| If the HTTPConnection is in the correct state, returns an | ||||||
| instance of HTTPResponse. | ||||||
| instance of HTTPResponse or of whatever object is returned by the | ||||||
| response_class variable. The connection will wait for a response other | ||||||
| than code 100 ('Continue') unless ignore_100_continue is set to False. | ||||||
|
Comment on lines
+1452
to
+1458
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "ignore..." is a somewhat of a negative term in English for use in an API which leads to awkward descriptions of the behavior. it defaults to True and people have to turn the negative off with False leading to double negatives to reason about. As this defaults to not supporting 100 continue behavior, why not call it (apply this everywhere) also we should make it keyword only so that it has to be spelled out when anyone wants to use it for better code readability. ( I also think eliding the number from the name is a good idea. 100 is defined as continue so no need to mention that internal http detail. |
||||||
|
|
||||||
| If a request has not been sent or if a previous response has | ||||||
| not be handled, ResponseNotReady is raised. If the HTTP | ||||||
|
|
@@ -1442,7 +1486,10 @@ def getresponse(self): | |||||
| if self.__state != _CS_REQ_SENT or self.__response: | ||||||
| raise ResponseNotReady(self.__state) | ||||||
|
|
||||||
| if self.debuglevel > 0: | ||||||
| if self.__pending_response: | ||||||
| response = self.__pending_response | ||||||
| self.__pending_response = None | ||||||
| elif self.debuglevel > 0: | ||||||
| response = self.response_class(self.sock, self.debuglevel, | ||||||
| method=self._method) | ||||||
| else: | ||||||
|
|
@@ -1451,21 +1498,24 @@ def getresponse(self): | |||||
| try: | ||||||
| try: | ||||||
| if self.max_response_headers is None: | ||||||
| response.begin() | ||||||
| response.begin(ignore_100_continue=ignore_100_continue) | ||||||
| else: | ||||||
| response.begin(_max_headers=self.max_response_headers) | ||||||
| response.begin(ignore_100_continue=ignore_100_continue, | ||||||
| _max_headers=self.max_response_headers) | ||||||
| except ConnectionError: | ||||||
| self.close() | ||||||
| raise | ||||||
| assert response.will_close != _UNKNOWN | ||||||
| self.__state = _CS_IDLE | ||||||
| if response.code != 100: | ||||||
| # Code 100 is effectively 'not a response' for this purpose | ||||||
| self.__state = _CS_IDLE | ||||||
|
|
||||||
| if response.will_close: | ||||||
| # this effectively passes the connection to the response | ||||||
| self.close() | ||||||
| else: | ||||||
| # remember this, so we can tell when it is complete | ||||||
| self.__response = response | ||||||
| if response.will_close: | ||||||
| # this effectively passes the connection to the response | ||||||
| self.close() | ||||||
| else: | ||||||
| # remember this, so we can tell when it is complete | ||||||
| self.__response = response | ||||||
|
|
||||||
| return response | ||||||
| except: | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't mention the version here. think of docs as being read in the future, version specific changes are normally footnotes via the version added and version changed markers. (this is a feature so it won't be in 3.15)