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

Restore performance of 304s after #10101 #10112

Closed
bdraco opened this issue Dec 5, 2024 · 2 comments · Fixed by #10113
Closed

Restore performance of 304s after #10101 #10112

bdraco opened this issue Dec 5, 2024 · 2 comments · Fixed by #10113

Comments

@bdraco
Copy link
Member

bdraco commented Dec 5, 2024

The fix certainly handles the closings smartly, but I would be more worried about the additional overhead of the file openings. If cache is controlled well, many implementations are likely serving 304s much more than 200s. #10101 probably introduced a minor penalty there, but I don't see a better way to fix this issue. 🤷🏻‍♂️

Originally posted by @steverep in #8013 (comment)

@bdraco
Copy link
Member Author

bdraco commented Dec 5, 2024

I think we could move the checks into the executor job as they are thread-safe already. Then return the state from executor job without the file open if its going to be a 304

@bdraco
Copy link
Member Author

bdraco commented Dec 5, 2024

ie this block

        etag_value = f"{st.st_mtime_ns:x}-{st.st_size:x}"
        last_modified = st.st_mtime

        # https://www.rfc-editor.org/rfc/rfc9110#section-13.1.1-2
        ifmatch = request.if_match
        if ifmatch is not None and not self._etag_match(
            etag_value, ifmatch, weak=False
        ):
            return await self._precondition_failed(request)

        unmodsince = request.if_unmodified_since
        if (
            unmodsince is not None
            and ifmatch is None
            and st.st_mtime > unmodsince.timestamp()
        ):
            return await self._precondition_failed(request)

        # https://www.rfc-editor.org/rfc/rfc9110#section-13.1.2-2
        ifnonematch = request.if_none_match
        if ifnonematch is not None and self._etag_match(
            etag_value, ifnonematch, weak=True
        ):
            return await self._not_modified(request, etag_value, last_modified)

        modsince = request.if_modified_since
        if (
            modsince is not None
            and ifnonematch is None
            and st.st_mtime <= modsince.timestamp()
        ):
            return await self._not_modified(request, etag_value, last_modified)

would become something like

        etag_value = f"{st.st_mtime_ns:x}-{st.st_size:x}"
        last_modified = st.st_mtime

        # https://www.rfc-editor.org/rfc/rfc9110#section-13.1.1-2
        ifmatch = request.if_match
        if ifmatch is not None and not self._etag_match(
            etag_value, ifmatch, weak=False
        ):
            return FileResponseState.PRE_CONDITION_FAILED

        unmodsince = request.if_unmodified_since
        if (
            unmodsince is not None
            and ifmatch is None
            and st.st_mtime > unmodsince.timestamp()
        ):
            return FileResponseState.PRE_CONDITION_FAILED

        # https://www.rfc-editor.org/rfc/rfc9110#section-13.1.2-2
        ifnonematch = request.if_none_match
        if ifnonematch is not None and self._etag_match(
            etag_value, ifnonematch, weak=True
        ):
            return FileResponseState.NOT_MODIFIED

        modsince = request.if_modified_since
        if (
            modsince is not None
            and ifnonematch is None
            and st.st_mtime <= modsince.timestamp()
        ):
            return FileResponseState.NOT_MODIFIED

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 a pull request may close this issue.

1 participant