gh-42550: Add 'Expect: 100-Continue' support to httplib by IngridMorstrad · Pull Request #133276 · python/cpython · GitHub
Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 85 additions & 5 deletions Doc/library/http.client.rst
92 changes: 71 additions & 21 deletions Lib/http/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
import http
import io
import re
import select
import socket
import sys
import collections.abc
Expand Down Expand Up @@ -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):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the * was important, it makes these keyword only arguments which are easier to support on public APIs like this method. (rule of thumb, when adding a new parameter it either has to be added at the end and if there are either a lot of arguments or are very infrequently passed should be added as keyword only so that it must be passed by name)

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)
Expand Down Expand Up @@ -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, *,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 HTTPSConnection.__init__.

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 Expect: 100-Continue header rather than expecting people to fill in that level of implementation detail header value directly themselves. (I think the code still needs detect if the header was added manually regardless, kind of like it does for some of the other headers)

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
Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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], [], [],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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'):
Expand Down Expand Up @@ -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
Expand All @@ -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):
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 support_continue=False so that enabling support for it involves setting something to True instead of setting an ignore= to False?

(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. (blah.getresponse(True) doesn't indicate much to the reader).

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
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down
Loading
Loading