GeezerVison

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

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

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

And pass http_server_kwargs to super()

@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.

You're already checking for is not None later

Let's group the parameters per purpose

You must also modify create_https_server appropriately

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

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.

I don't think we should add this information. There is no notion of an "instance argument": it should rather be an instance attribute, and this should be documented through a .. attribute::, below .. attribute:: extensions_map

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:

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.

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.

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

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.

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

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.

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?

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

Clarify as an internal private attribute:

Or document SimpleHTTPRequestHandler.response_headers as a public attribute.

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?

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.

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

Did we consider using not hasattr(...) check as for _headers_buffer? it's mostly for a follow-up/refactoring. If we go that path, we would remove the _default_response_headers = [] in handle() and put it in flush_headers() instead, right? It would re-construct the default_response_headers for each request but this also mean that if a request does (in the future) send_header(..., is_extra=True), that is_extra is "local" to the current request and won't pollute next requests. For now it doesn't matter though but I'll write it here to remember.

I actually didn't look at the pattern used for _headers_buffer. For consistency and correctness, yes it does seem like the right thing to do it that way. I've made that change in 597b973.

Ah, maybe this can hang. I don't know if this test was written as for other tests, but this may hang in some build bots. The server_thread.join call below should be added as a finalizer just in case. And we should have a short timeout before aborting the thread.

Ok, I added some addCleanup calls in c40f8b1 to help prevent a hang. If you think there's still a risk of the test hanging, I can just remove the test for now until we come up with a better idea.

I still saw some resource leaking, so I just ended up removing test_extra_response_headers_concurrent_requests in 347b968 . We can add a more robust test later if need be, but I didn't want to leave a flaky test in that risked breaking.

After that checkin, there was still an env changed leakage in a windows build. I refactored a different test test_extra_response_headers_arg in a249eea to not create a server and thus also less likely to leak. After that checkin, all tests are passing.