Optimize Fresh header parsing for fasthttp 1.65 #3687
Conversation
WalkthroughRefactors Fresh() in req.go to use fasthttp headers and date parsing, removes net/http dependency, switches header lookups to byte slices, narrows variable scope, passes Cache-Control via utils.UnsafeString, and updates ETag and Last-Modified comparisons through isEtagStale and fasthttp.ParseHTTPDate. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Fresh as Fresh()
participant ReqHdr as Request.Header
participant ResHdr as Response.Header
participant Utils as utils/isNoCache
participant ETag as isEtagStale
Caller->>Fresh: invoke
Fresh->>ReqHdr: Peek If-None-Match, If-Modified-Since, Cache-Control
Fresh->>Utils: isNoCache(UnsafeString(Cache-Control))
Utils-->>Fresh: no-cache?
alt no-cache
Fresh-->>Caller: false
else check ETag
Fresh->>ETag: isEtagStale(noneMatch []byte, res ETag)
ETag-->>Fresh: stale?
alt not stale
Fresh-->>Caller: true
else check dates
Fresh->>ResHdr: Peek Last-Modified
Fresh->>ReqHdr: Peek If-Modified-Since
Fresh-->>Caller: lastModified <= ifModifiedSince ?
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ReneWerner87, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on optimizing the Fresh() method's header parsing within the fasthttp context. By directly peeking request headers and leveraging fasthttp's native date parsing, it aims to significantly improve performance when determining if a cached response is still fresh, particularly for If-Modified-Since and If-None-Match checks.
Highlights
- Performance Enhancement: The Fresh() method now directly accesses request headers using header.Peek() for If-Modified-Since, If-None-Match, and Cache-Control. This avoids the overhead of generic header retrieval, leading to faster processing.
- Optimized Date Parsing: Date string parsing for Last-Modified and If-Modified-Since headers has been switched from http.ParseTime to fasthttp.ParseHTTPDate, utilizing fasthttp's more efficient, specialized parsing utility.
- Reduced Dependencies: The net/http package import has been removed from req.go, streamlining dependencies as its ParseTime function is no longer required.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the Fresh header parsing functionality for fasthttp 1.65 by switching from generic header parsing methods to direct header peeking operations. The optimization aims to improve performance by reducing string conversions and using fasthttp's built-in HTTP date parsing.
- Replace generic header parsing with direct
header.Peek()operations - Switch from
http.ParseTimetofasthttp.ParseHTTPDatefor date parsing - Optimize ETag matching by working directly with byte arrays
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3687 +/- ##
=======================================
Coverage 92.01% 92.01%
=======================================
Files 114 114
Lines 11494 11495 +1
=======================================
+ Hits 10576 10577 +1
Misses 659 659
Partials 259 259
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
https://github.com//pull/3684#pullrequestreview-3121936641
|
There was a problem hiding this comment.
Code Review
This pull request aims to optimize the Fresh() method by using fasthttp's direct header parsing, which is a good performance enhancement. However, I've identified a critical logic flaw in how the If-None-Match and If-Modified-Since headers are handled. The current implementation can lead to incorrect cache validation, particularly when only the If-Modified-Since header is present. I've provided a detailed comment with a suggested code change to fix this logical issue while preserving the performance gains.
before 1.65.0 after the merge in this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
req.go (3)
231-246: Use r.Request() consistently and clarify variable naming (nit).For consistency with the rest of the file (e.g., Lines 35, 41, 47, 61), prefer using r.Request() instead of r.c.Request(). Also, naming the variable reqHeader improves readability over a generic header name.
No functional change; purely stylistic/readability.
Apply this diff:
- header := &r.c.Request().Header + reqHeader := &r.Request().Header @@ - modifiedSince := header.Peek(HeaderIfModifiedSince) - noneMatch := header.Peek(HeaderIfNoneMatch) + modifiedSince := reqHeader.Peek(HeaderIfModifiedSince) + noneMatch := reqHeader.Peek(HeaderIfNoneMatch) @@ - cacheControl := header.Peek(HeaderCacheControl) - if len(cacheControl) > 0 && isNoCache(utils.UnsafeString(cacheControl)) { + cacheControl := reqHeader.Peek(HeaderCacheControl) + if len(cacheControl) > 0 && isNoCache(utils.UnsafeString(cacheControl)) { return false }
262-275: Use time.After to avoid potential Go version dependency on time.Time.Compare.The current comparison:
- Line 273: return lastModifiedTime.Compare(modifiedSinceTime) != 1
is equivalent to “lastModified <= modifiedSince.” Compare was added in newer Go versions; using After is widely compatible and clearer.
Apply this diff:
- return lastModifiedTime.Compare(modifiedSinceTime) != 1 + return !lastModifiedTime.After(modifiedSinceTime)If the repo needs to support older Go versions, this avoids requiring a recent standard library. Please confirm the minimum Go version; if Compare is acceptable in your target, feel free to keep it.
251-278: Add targeted tests for edge cases (If-None-Match "*" and combined validators).To lock in behavior and guard against regressions, add tests covering:
- If-None-Match: "*" (with and without Last-Modified) → expect Fresh() == true unless Cache-Control: no-cache is present.
- Matching ETag plus If-Modified-Since where Last-Modified > If-Modified-Since → expect Fresh() == false.
- Matching ETag plus If-Modified-Since where Last-Modified <= If-Modified-Since → expect Fresh() == true.
- Only If-Modified-Since present with valid Last-Modified → expect Fresh() true iff Last-Modified <= If-Modified-Since.
- Cache-Control: no-cache present → expect Fresh() == false regardless of other headers.
I can draft these tests in the existing test suite style if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
req.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
req.go (2)
constants.go (5)
HeaderIfModifiedSince(183-183)HeaderIfNoneMatch(184-184)HeaderCacheControl(168-168)HeaderETag(181-181)HeaderLastModified(186-186)client/response.go (1)
Response(19-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, macos-latest)
- GitHub Check: unit (1.25.x, macos-13)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: lint
🔇 Additional comments (3)
req.go (3)
238-241: Correct unconditional-request short-circuit.Returning false when neither If-Modified-Since nor If-None-Match are present matches HTTP caching semantics and the jshttp/fresh behavior.
245-247: No-cache handling is correct and allocation-free.Short-circuiting on Cache-Control: no-cache and passing utils.UnsafeString(cacheControl) to isNoCache avoids intermediate allocations. Ensure isNoCache does not retain the string beyond the call (it shouldn’t), to keep the UnsafeString usage safe.
Would you confirm isNoCache does not store the passed string? If unsure, I can scan the codebase for its usages.
251-259: ETag matching path is correct and tight.
- Skipping the block for If-None-Match: "*" and returning true later aligns with conditional request semantics.
- Narrowing app’s scope is a tidy improvement.
- Passing noneMatch as []byte directly into app.isEtagStale avoids extra conversions.
Summary
Freshby peeking request headers directly and bypassing generic parsingfasthttp.ParseHTTPDateTesting
go test -run=^$ -bench=Benchmark_Ctx_Fresh_LastModified -benchmem -count=1make audit(fails: EncodeMsg passes lock by value)make generatemake betteralign(fails: package requires newer Go version go1.25)make modernize(fails: package requires newer Go version go1.25)make formatmake test(fails: Test_App_BodyLimit_LargerThanDefault i/o timeout)https://chatgpt.com/codex/tasks/task_e_689e46119e348326ac769a9a3d21acd8