Skip to content
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

Use updated headers from server state #1706

Merged
merged 1 commit into from Oct 19, 2022
Merged

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Oct 12, 2022

Does anyone know a way to test this feature? ;)

Testing with asyncio.sleep is not reliable.

Related PRs

This was already worked, and reverted twice. I'm sorry for that.

This PR is a blocker for the next release.

@Kludex Kludex requested a review from a team October 12, 2022 08:27
@Kludex Kludex added this to the Version 0.19.0 milestone Oct 12, 2022
@tomchristie
Copy link
Member

Does anyone know a way to test this feature?

For a first pass can we just describe the change in behaviour against this issue, with a manual demonstration.
I assume this change resolves #1618, right?

@Kludex
Copy link
Sponsor Member Author

Kludex commented Oct 12, 2022

Application:

async def app(scope, receive, send):
    await send(
        {
            "type": "http.response.start",
            "status": 200,
            "headers": [
                [b"content-type", b"text/plain"],
            ],
        }
    )
    await send(
        {
            "type": "http.response.body",
            "body": b"Hello, world!",
        }
    )

Server command (set timeout-keep-alive to make sure the connection stays alive):

uvicorn main:app --reload --timeout-keep-alive 100

Client:

from time import sleep

from httpx import Client

with Client() as client:
    response = client.get("http://localhost:8000")
    first_date_header = response.headers["date"]

    sleep(3)

    response = client.get("http://localhost:8000")
    second_date_header = response.headers["date"]
    assert first_date_header != second_date_header, "Date headers should be different"

You can test the code with and without this PR. 🙏

@Kludex
Copy link
Sponsor Member Author

Kludex commented Oct 12, 2022

Does anyone know a way to test this feature?

For a first pass can we just describe the change in behaviour against this issue, with a manual demonstration. I assume this change resolves #1618, right?

Correct. It solves #1618.

@tomchristie
Copy link
Member

Pragmatically I'd be inclined to just accept this pull request.

@euri10
Copy link
Member

euri10 commented Oct 12, 2022

as far as I'm ocncerned I'm not inclined in accepting it without a test. This is a reather simple test, just sending 2 queries waiting 1s in between, there's no magic at all, so the test fails smells really bad, the fact the sleep is not respected in the CI looks bad as well, if the logs displayed are correct of course, the fact noone here can understand why it fails is probably the worse.

anyways, the fix looks good, you can merge it without a test, but this will resurface one way or the other because it's highly illogical it's failing at the moment.

@tomchristie
Copy link
Member

tomchristie commented Oct 12, 2022

noone here can understand why it fails is probably the worse

We know why it fails, and why this is the right fix...

The ServerState holds the default headers that should be used, and is updated ~once per second, which includes setting the Date field...

uvicorn/uvicorn/server.py

Lines 224 to 250 in a94781d

async def on_tick(self, counter: int) -> bool:
# Update the default headers, once per second.
if counter % 10 == 0:
current_time = time.time()
current_date = formatdate(current_time, usegmt=True).encode()
if self.config.date_header:
date_header = [(b"date", current_date)]
else:
date_header = []
self.server_state.default_headers = (
date_header + self.config.encoded_headers
)
# Callback to `callback_notify` once every `timeout_notify` seconds.
if self.config.callback_notify is not None:
if current_time - self.last_notified > self.config.timeout_notify:
self.last_notified = current_time
await self.config.callback_notify()
# Determine if we should exit.
if self.should_exit:
return True
if self.config.limit_max_requests is not None:
return self.server_state.total_requests >= self.config.limit_max_requests
return False

This PR changes where we include the ServerState.default_headers.
Instead of being read once at the start of the connection, they're instead read for every request.

It's a simple fix. It's just awkward to write a test for is all.

@euri10
Copy link
Member

euri10 commented Oct 13, 2022

noone here can understand why it fails is probably the worse

We know why it fails, and why this is the right fix...

I'm not debating the fix, which is good, I was talking about the test that fails.
Anyways do as you want.

@Kludex
Copy link
Sponsor Member Author

Kludex commented Oct 13, 2022

The problem on the test is that comparing time with time.time can lead to this lack of precision.

I can mock patch the datetime format on the date header, and also mock patch time.time to use loop.time, and then making sure that is bigger than one second and less than two...

But I guess things become too complex?

@Kludex
Copy link
Sponsor Member Author

Kludex commented Oct 19, 2022

@tomchristie @euri10 Can I have an approval here to proceed with the release? 🙏

In any case, I'll try to increase the test coverage to 100% until the end of the year (I'm not sure if I can cover this scenario anyway).

@Kludex Kludex mentioned this pull request Oct 19, 2022
5 tasks
Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

Absolutely, yup. 👍

@tomchristie tomchristie merged commit 001e7d7 into master Oct 19, 2022
15 checks passed
@tomchristie tomchristie deleted the fix/date-headers-update3 branch October 19, 2022 08:19
Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
Kludex added a commit that referenced this pull request Oct 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Date" header not changing between requests
3 participants