Skip to content

gh-135056: Add a --header CLI argument to http.server #135057

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

aisipos
Copy link

@aisipos aisipos commented Jun 3, 2025

As proposed in #135056, Add a --cors command line argument to the stdlib http.server module, which will add an Access-Control-Allow-Origin: * header to all responses.

Invocation:

python -m http.server --cors

As part of this implementation, add a response_headers argument to SimpleHTTPRequestHandler and HTTPServer, which allows callers to add addition headers to the response. Ideally it would have been possible to just have made a CorsHttpServer class, but a couple of issues made that difficult:

  • The http.server CLI uses more than one HTTP Server class, in order to support TLS/HTTPS. So a single CorsHttpServer child class wouldn't work to support both use cases.
  • Much of the work in specifying HTTP behavior is handled by the various RequestHandler classes. However, the HttpServer classes didn't have an easy way to pass arguments down into the instantiated handlers.

As a result, this PR updates both HTTPServer and SimpleHTTPRequestHandler to accept a response_headers argument, which allows callers to specify an additional set of HTTP headers to pass in the response.

  • HTTPServer now overrides finish_request to pass this new kwarg down to its RequestHandler.
  • SimpleHTTPRequestHandler now accepts a resposnse_headers kwarg, to optionally specify a dictionary of additional headers to send in the response.

Care is taken to not pass the response_headers argument to any instance constructors when not provided, to ensure backwards compatibility. I tried to keep the implementation as short and simple as possible.

With the addition of a response_headers argument, we allow ourselves to have a future possible custom header http argument, such as:

python -m http.server -H 'custom-header: custom-value'

📚 Documentation preview 📚: https://cpython-previews--135057.org.readthedocs.build/

@python-cla-bot
Copy link

python-cla-bot bot commented Jun 3, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Jun 3, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Add a --cors command line argument to the stdlib http.server module, which will
add an `Access-Control-Allow-Origin: *` header to all responses. As part of this
implementation, add a `response_headers` argument to SimpleHTTPRequestHandler
and HttpServer, which allows callers to add addition headers to the response.
@aisipos aisipos force-pushed the https-server-cors-issue-135056 branch from 3f11652 to 0d02fbe Compare June 3, 2025 05:24
Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

(I'd prefer a general headers option, but will comment on the issue or Discourse topic)

@aisipos
Copy link
Author

aisipos commented Jun 3, 2025

test_wsgiref is failing. I'll look into it.

This fixes the breakage to HttpServer as used by wsgiref.
@aisipos
Copy link
Author

aisipos commented Jun 3, 2025

test_wsgiref fixed in a3256fd. This should fix any backwards incompatibility errors erroneously introduced in the first commit.

@donBarbos
Copy link
Contributor

I think it's worth adding to this What's New entry (./Doc/whatsnew/3.15.rst)

@Zheaoli
Copy link
Contributor

Zheaoli commented Jun 6, 2025

For me, I don't think add --cors CLI argument would be a good idea. Base on following reasons:

  1. The CORS policy is a complicated idea. Six response headers are included by the word(If I'm correct). If you set the Access-Control-Allow-Origin, and now the people may need Access-Control-Allow-Methods( Allow for OPTION method). What is the next argument we need to add?
  2. The CLI for http.server is just for a debug target. So we design the CLI as simple as we can. The developer don't need to care about any extra detail when they run just a simple debug server.
  3. if we need CORS policy in the future. I suggest we setup all the header for developer and don't need to add cli for it.

@aisipos aisipos requested a review from hugovk June 6, 2025 23:05
@picnixz picnixz self-requested a review June 7, 2025 23:15
@hugovk
Copy link
Member

hugovk commented Jun 9, 2025

@hugovk
Copy link
Member

hugovk commented Jun 9, 2025

(I'd prefer a general headers option, but will comment on the issue or Discourse topic)

https://discuss.python.org/t/any-interest-in-adding-a-cors-option-to-python-m-http-server/92120/24

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I'm not very fond of how the HTTP server class is growing more and more with more __init__ parameters, but I don't have a better idea for now. Maybe a generic configuration object but this would be an overkill for this class in particular I think.

@@ -543,6 +553,14 @@ The following options are accepted:

.. versionadded:: 3.14

.. option:: --cors
Copy link
Member

Choose a reason for hiding this comment

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

As Hugo said, since we're anyway exposing response-headers, I think we should also expose it from the CLI. It could be useful for users in general (e.g., --add-header NAME VALUE with the -H alias).

@@ -0,0 +1,2 @@
Add a ``--cors`` cli option to :program:`python -m http.server`. Contributed by
Copy link
Member

Choose a reason for hiding this comment

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

Let's also update What's New/3.15.rst

Copy link
Author

Choose a reason for hiding this comment

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

I used blurb to make this entry in NEWS.d, not knowing when it's appropriate to edit the main 3.15.rst file. I think once we know if we're doing --cors / --header , or both, I can make the appropriate update to What's New/3.15.rst

@@ -132,7 +144,7 @@ class ThreadingHTTPServer(socketserver.ThreadingMixIn, HTTPServer):
class HTTPSServer(HTTPServer):
def __init__(self, server_address, RequestHandlerClass,
bind_and_activate=True, *, certfile, keyfile=None,
password=None, alpn_protocols=None):
password=None, alpn_protocols=None, response_headers=None):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
password=None, alpn_protocols=None, response_headers=None):
password=None, alpn_protocols=None, **http_server_kwargs):

And pass http_server_kwargs to super()

Comment on lines 133 to 138
args = (request, client_address, self)
kwargs = {}
response_headers = getattr(self, 'response_headers', None)
if response_headers:
kwargs['response_headers'] = self.response_headers
self.RequestHandlerClass(*args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
args = (request, client_address, self)
kwargs = {}
response_headers = getattr(self, 'response_headers', None)
if response_headers:
kwargs['response_headers'] = self.response_headers
self.RequestHandlerClass(*args, **kwargs)
kwargs = {}
if hasattr(self, 'response_headers'):
kwargs['response_headers'] = self.response_headers
self.RequestHandlerClass(request, client_address, self, **kwargs)

Copy link
Author

Choose a reason for hiding this comment

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

@picnixz I made this requested change in 77b5fff. Note though that now HTTPServer will pass response_headers to the RequestHandler class even if response_headers is None or {}. Most RequestHandler implementation constructor implementations don't take this argument, so in order for this to work I added **kwargs as an argument to BaseRequestHandler.__init__. My earlier implementation was trying to prevent this, to keep any changes to only http/server.py and not need to touch anything in socketserver.py. Perhaps the **kwargs addition is ok, or I'm open to other solutions if we can think of better ones.

Comment on lines 708 to 712
def __init__(self, *args, directory=None, response_headers=None, **kwargs):
if directory is None:
directory = os.getcwd()
self.directory = os.fspath(directory)
self.response_headers = response_headers or {}
Copy link
Member

@picnixz picnixz Jun 9, 2025

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, *args, directory=None, response_headers=None, **kwargs):
if directory is None:
directory = os.getcwd()
self.directory = os.fspath(directory)
self.response_headers = response_headers or {}
def __init__(self, *args, directory=None, response_headers=None, **kwargs):
if directory is None:
directory = os.getcwd()
self.directory = os.fspath(directory)
self.response_headers = response_headers

You're already checking for is not None later

@@ -970,7 +991,7 @@ def _get_best_family(*address):
def test(HandlerClass=BaseHTTPRequestHandler,
ServerClass=ThreadingHTTPServer,
protocol="HTTP/1.0", port=8000, bind=None,
tls_cert=None, tls_key=None, tls_password=None):
tls_cert=None, tls_key=None, tls_password=None, response_headers=None):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tls_cert=None, tls_key=None, tls_password=None, response_headers=None):
tls_cert=None, tls_key=None, tls_password=None,
response_headers=None):

Let's group the parameters per purpose

Comment on lines 1078 to 1082
handler_args = (request, client_address, self)
handler_kwargs = dict(directory=args.directory)
if self.response_headers:
handler_kwargs['response_headers'] = self.response_headers
self.RequestHandlerClass(*handler_args, **handler_kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
handler_args = (request, client_address, self)
handler_kwargs = dict(directory=args.directory)
if self.response_headers:
handler_kwargs['response_headers'] = self.response_headers
self.RequestHandlerClass(*handler_args, **handler_kwargs)
self.RequestHandlerClass(request, client_address, self,
directory=args.directory,
response_headers=self.response_headers)

@@ -95,7 +96,8 @@ def run(self):
request_handler=self.request_handler,
)
else:
self.server = HTTPServer(('localhost', 0), self.request_handler)
self.server = HTTPServer(('localhost', 0), self.request_handler,
Copy link
Member

Choose a reason for hiding this comment

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

You must also modify create_https_server appropriately

Comment on lines 832 to 834
server_kwargs = dict(
response_headers = {'Access-Control-Allow-Origin': '*'}
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
server_kwargs = dict(
response_headers = {'Access-Control-Allow-Origin': '*'}
)
server_kwargs = {
'response_headers': {'Access-Control-Allow-Origin': '*'}
}

server_kwargs = dict(
response_headers = {'Access-Control-Allow-Origin': '*'}
)
def test_cors(self):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_cors(self):
def test_cors(self):

@aisipos aisipos force-pushed the https-server-cors-issue-135056 branch from 3024d3d to 5f89c97 Compare June 20, 2025 01:50
@aisipos
Copy link
Author

aisipos commented Jun 20, 2025

@picnixz I have made all your suggested changes in 77b5fff . I have also implemented a generic -H or --header cli argument in 5f89c97. Now this PR contains both --cors and --header. I don't know if we want both, there doesn't seem to yet be consensus on what we prefer, although Core devs so far seem to lean on just --header.

@hugovk
Copy link
Member

hugovk commented Jun 20, 2025

I think we should just have --header, as that unlocks the ability to enable CORS. We can still add --cors later if there's demand and consensus.

@hugovk
Copy link
Member

hugovk commented Jun 20, 2025

And what are your thoughts on positional args like HTTPie?

https://discuss.python.org/t/any-interest-in-adding-a-cors-option-to-python-m-http-server/92120/24

@@ -437,6 +444,9 @@ instantiation, of which this module provides three different variants:
.. versionchanged:: 3.7
Support of the ``'If-Modified-Since'`` header.

.. versionchanged:: next
Support ``response_headers`` as an instance argument.
Copy link
Member

Choose a reason for hiding this comment

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

Isn’t this redundant with the entry already under the constructor heading?

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps - it seems the constructor documentation is used to make a brief mention of each argument and when it was added, with more detail being filled in later. My latest commits make several changes requested elsewhere for other reasons, but if the current version is still too redundant in multiple places I can make some more edits.

@@ -1052,14 +1081,21 @@ def server_bind(self):

def finish_request(self, request, client_address):
self.RequestHandlerClass(request, client_address, self,
directory=args.directory)
directory=args.directory,
response_headers=self.response_headers)
Copy link
Member

Choose a reason for hiding this comment

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

Why not do this the same way the --directory or --protocol options are implemented? Either way should avoid adding internal parameters to unrelated HTTPServer and BaseRequestHandler classes.

You could build the response_headers dictionary before the DualStackServerMixin class, and then pass it by referencing the outer scope like is already done with args.directory:

Suggested change
response_headers=self.response_headers)
response_headers=response_headers)

Or set the response_headers attribute on the SimpleHTTPRequestHandler class rather than in its constructor, like is done for protocol_verison in the test function.

Copy link
Author

Choose a reason for hiding this comment

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

I followed your advice, see b1026d2. I made response_headers an argument to SimpleHTTPRequestHandler only, and send the argument to it in the DualStackServerMixin class.

@@ -374,6 +374,10 @@ instantiation, of which this module provides three different variants:
.. versionchanged:: 3.9
The *directory* parameter accepts a :term:`path-like object`.

.. versionchanged:: next
The *response_headers* parameter accepts an optional dictionary of
Copy link
Member

Choose a reason for hiding this comment

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

In previous versions, this was not a valid parameter at all.

Suggested change
The *response_headers* parameter accepts an optional dictionary of
Added *response_headers*, which accepts an optional dictionary of

Also, did you consider accepting a list or iterable of (name, value) pairs instead, like returned by http.client.HTTPResponse.getheaders? That would be better for sending multiple Set-Cookie fields.

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes, sending multiple headers of the same name would indeed be necessary. I updated to use an iterable of name value pairs instead in 7a793f2

@@ -374,6 +374,10 @@ instantiation, of which this module provides three different variants:
.. versionchanged:: 3.9
The *directory* parameter accepts a :term:`path-like object`.

.. versionchanged:: next
The *response_headers* parameter accepts an optional dictionary of
additional HTTP headers to add to each response.
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth clarifying how these fields interact with other fields such as Server specified under BaseHTTPRequestHandler.send_response, and Last-Modified under do_GET.

Also clarify which responses the fields are included in, assuming it wasn’t your intention to include them for 404 Not Found, 304 Not Modified, lower-level errors, etc.

Copy link
Author

Choose a reason for hiding this comment

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

In the latest commits, I've noted that the custom headers are only sent in success cases. What do you mean by interaction though? The custom headers currently get sent after Last-Modified, should I mention the placement of the custom headers and that they appear after Last-Modified?

Comment on lines 754 to 756
if self.response_headers is not None:
for header, value in self.response_headers.items():
self.send_header(header, value)
Copy link
Member

Choose a reason for hiding this comment

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

Or is moving this to an extended send_response override an option? That way you would include the fields for all responses.

if directory is None:
directory = os.getcwd()
self.directory = os.fspath(directory)
self.response_headers = response_headers
Copy link
Member

Choose a reason for hiding this comment

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

Clarify as an internal private attribute:

Suggested change
self.response_headers = response_headers
self._response_headers = response_headers

Or document SimpleHTTPRequestHandler.response_headers as a public attribute.

Copy link
Author

Choose a reason for hiding this comment

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

So far I have intended response_headers to be a public attribute. Do you mean add documentation to the docstring of SimpleHTTPRequestHandler or more documentation in Doc/library/http.server.rst?

)
else:
self.server = HTTPServer(('localhost', 0), self.request_handler)
self.server = HTTPServer(('localhost', 0), self.request_handler,
**self.server_kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

This appears to only be testing the undocumented or internal HTTPServer parameter. It would be good to test the new documented SimpleHTTPRequestHandler parameter instead or as well.

Copy link
Author

Choose a reason for hiding this comment

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

I have removed server_kwargs in the latest updates, and updated the tests. The only external change now is response_headers as an instance arg to SimpleHTTPRequestHandler

@aisipos aisipos requested a review from vadmium July 10, 2025 07:22
@@ -1024,6 +1035,11 @@ def _main(args=None):
parser.add_argument('port', default=8000, type=int, nargs='?',
help='bind to this port '
'(default: %(default)s)')
parser.add_argument('-H', '--header', nargs=2, action='append',
# metavar='HEADER VALUE',
Copy link
Member

Choose a reason for hiding this comment

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

Remove commented code:

Suggested change
# metavar='HEADER VALUE',

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 9450b86

Comment on lines 1493 to 1498
# with mock.patch.object(
# SimpleHTTPRequestHandler, '__init__'
# ) as mock_handler, \
# mock.patch.object(
# HTTPServer, 'serve_forever'
# ) as mock_serve_forever:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# with mock.patch.object(
# SimpleHTTPRequestHandler, '__init__'
# ) as mock_handler, \
# mock.patch.object(
# HTTPServer, 'serve_forever'
# ) as mock_serve_forever:

Copy link
Author

@aisipos aisipos Jul 15, 2025

Choose a reason for hiding this comment

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

Fixed in 9450b86. Sorry to mistakenly leave these in my earlier checkins.

Comment on lines 1059 to 1062
response_headers = []
for header, value in args.header or []:
response_headers.append((header, value))

Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be a list of tuples?

Or can it be a list of lists? If so, we can use args.header directly:

Suggested change
response_headers = []
for header, value in args.header or []:
response_headers.append((header, value))

Copy link
Author

Choose a reason for hiding this comment

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

It can be lists, so yes using the args.header value directly is a good idea, thanks! Updated in d317cc2

@@ -1052,7 +1072,8 @@ def server_bind(self):

def finish_request(self, request, client_address):
self.RequestHandlerClass(request, client_address, self,
directory=args.directory)
directory=args.directory,
response_headers=response_headers)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
response_headers=response_headers)
response_headers=args.header)

Copy link
Author

Choose a reason for hiding this comment

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

Updated as above in d317cc2

@@ -0,0 +1,2 @@
Add a ``--header`` cli option to :program:`python -m http.server`. Contributed by
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Add a ``--header`` cli option to :program:`python -m http.server`. Contributed by
Add a ``--header`` CLI option to :program:`python -m http.server`. Contributed by

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 5a30d91

Comment on lines 1499 to 1501
with mock.patch.object(
HTTPServer, 'serve_forever'
) as mock_serve_forever:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with mock.patch.object(
HTTPServer, 'serve_forever'
) as mock_serve_forever:
with mock.patch.object(HTTPServer, 'serve_forever'):

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 9450b86

@aisipos aisipos requested a review from hugovk July 15, 2025 04:35
@aisipos aisipos requested a review from picnixz August 8, 2025 03:18
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

We're still lacking a What's New entry (Doc/whatsnew/3.15.rst). I would appreciate that the test and _main() functions are rewritten: test() should only serve for ever, _main() should only call test()and a new_make_serverfunction should just instantiate the server without callingserve_forever`:

def _make_server(...):
    server = ServerClass(...)
    return server

If needed, you can also return host, port and server to ease your life in the tests. But ideally it's better to split functions' responsibilities.

@@ -362,7 +362,7 @@ instantiation, of which this module provides three different variants:
delays, it now always returns the IP address.


.. class:: SimpleHTTPRequestHandler(request, client_address, server, directory=None)
.. class:: SimpleHTTPRequestHandler(request, client_address, server, directory=None, response_headers=None)
Copy link
Member

Choose a reason for hiding this comment

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

Document that directory and response_headers are keyword arguments actually.

Copy link
Author

Choose a reason for hiding this comment

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

Done in c376a71

@@ -713,6 +714,13 @@ def do_HEAD(self):
if f:
f.close()

def send_custom_response_headers(self):
Copy link
Member

Choose a reason for hiding this comment

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

Should we simply have "send_response_headers"? or is there a need to add "custom_response_headers"? is there a plan for a future distinction between these two?

Copy link
Author

Choose a reason for hiding this comment

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

I added this function to DRY the usages in both send_head and list_directory. I felt that the check for response_headers being None was a hidden implementation detail that I wanted to hide.

I suppose it would be possible to combine the functionality of the other headers into a single send_response_headers, but I think there are downsides to this. There is at least one slight difference in that send_head is sending a Last-Modified header and list-directory isn't. So we'd either have to:

  • Send a set of pre-computed headers to a send_response_headers function. In this case it doesn't feel like it's doing a better job than individual calls to send_header like there is now.
  • Send enough parameters to have a send_response_headers function to figure them out. This feels a bit "inverted" to me, it feels like the caller wouldn't want to delegate this responsibility away, the caller already knows enough to determine this.

I do like the distinct naming of custom_response_headers in this function, because it highlights that these might be headers beyond just what the HTTP standard is calling for. Perhaps I should have used this name as the kwarg to SimpleHTTPRequestHandler for consistency. Perhaps also the name user_defined_headers is better. Naming things is hard of course, the names seemed to make sense to me as I wrote them, but if you think the naming conventions are confusing I'm open to other ideas.

parser.add_argument('-H', '--header', nargs=2, action='append',
metavar=('HEADER', 'VALUE'),
help='Add a custom response header '
'(can be used multiple times)')
Copy link
Member

Choose a reason for hiding this comment

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

The can be used multiple times is already implied with `action='append' I think.

Copy link
Author

Choose a reason for hiding this comment

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

I agree in the code the multiple hint is obvious, but I added this for users running --help in the CLI, which will show with python -m http.server --help:

  -H, --header HEADER VALUE
                        Add a custom response header (can be used multiple times)

argparse doesn't give us an indicator in the --help output automatically for action-'append', so I added an explicit hint.

@@ -998,6 +1008,7 @@ def test(HandlerClass=BaseHTTPRequestHandler,
except KeyboardInterrupt:
print("\nKeyboard interrupt received, exiting.")
sys.exit(0)
return server
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this. The test function only "serves" the server and exits upon Ctrl+C. It's not meant to return the server instance. Instead, I suggest we have a function that constructs the server and test would become:

with _make_server(...) as httpd:
    print(...)
    try:
        httpd.serve_forever()
     except ...
``

In the tests you can directly use `_make_server`.

Copy link
Author

Choose a reason for hiding this comment

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

In 5f1fb94 I used your idea to factor out a _make_server function, and used the wraps feature of mock.patch to use it in the tests to create the server / handler class. I then removed the return values from test and main.

@@ -1061,7 +1077,7 @@ class HTTPSDualStackServer(DualStackServerMixin, ThreadingHTTPSServer):

ServerClass = HTTPSDualStackServer if args.tls_cert else HTTPDualStackServer

test(
return test(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return test(
test(

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 5f1fb94

Comment on lines 1498 to 1500
with mock.patch.object(
request_handler_class, '__init__'
) as mock_handler_init:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
with mock.patch.object(
request_handler_class, '__init__'
) as mock_handler_init:
with mock.patch.object(
request_handler_class, '__init__'
) as mock_handler_init:

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 5f1fb94

@hugovk hugovk changed the title gh-135056: Add a --cors CLI argument to http.server gh-135056: Add a --header CLI argument to http.server Aug 8, 2025
@aisipos aisipos requested a review from AA-Turner as a code owner August 12, 2025 06:15
@aisipos
Copy link
Author

aisipos commented Aug 12, 2025

We're still lacking a What's New entry (Doc/whatsnew/3.15.rst).

I added What's new entries in 89a89f0

@aisipos aisipos requested a review from picnixz August 12, 2025 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants