release: v0.1.0#20
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request hardens the Golpher web framework for production by synchronizing route registration, freezing the route table on the first request, tracking response committed state, masking unknown errors, and making request body reading lazy and bounded. It also unifies the handler API to a single HandlerFunc signature and introduces first-class support for the HTTP QUERY method (RFC 10008). The review feedback identifies critical issues, including an observability regression in UseHTTP where standard middleware completes before error responses are written, a potential memory leak in pooled requests due to uncleared parameter values, and an invalid "//" static route registration when the root path is registered. Additionally, it recommends updating test assertions to verify correct error status observation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| var handlerErr error | ||
| h := middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| handlerErr = next(req, res) | ||
| })) | ||
| h.ServeHTTP(res.Raw(), req.http) | ||
| if handlerErr != nil { | ||
| app.reportError(req, res, handlerErr) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Standard Middleware Observability Regression
In the current implementation, app.reportError is called after the adapted standard middleware's ServeHTTP method returns:
h.ServeHTTP(res.Raw(), req.http)
if handlerErr != nil {
app.reportError(req, res, handlerErr)
}This means any standard logging, metrics, or response-modifying middleware (e.g., gzip, Prometheus metrics, Apache loggers) wrapped via UseHTTP will execute and complete before the error response (status code and body) is actually written to the wire. Consequently, they will observe an unwritten response (defaulting to 200 OK with 0 bytes), leading to incorrect logs and metrics.
To restore full interoperability with standard middleware, we should invoke app.reportError inside the http.HandlerFunc callback if next returns an error. This ensures the error response is fully written before the standard middleware's ServeHTTP returns, while setting handlerErr = nil prevents double-reporting.
var handlerErr error
h := middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
handlerErr = next(req, res)
if handlerErr != nil {
app.reportError(req, res, handlerErr)
handlerErr = nil
}
}))
h.ServeHTTP(res.Raw(), req.http)
if handlerErr != nil {
app.reportError(req, res, handlerErr)
}
return nil
}| request.http = nil | ||
| request.params = nil | ||
| request.paramNames = nil | ||
| request.paramValues = request.paramValues[:0] |
There was a problem hiding this comment.
Prevent Reference Retention / Memory Leak in Pooled Requests
request.paramValues is a slice of strings. When resetting it via request.paramValues = request.paramValues[:0], the elements beyond the new length (0) are not cleared in the underlying array. Because Request structs are pooled in requestPool (sync.Pool), references to these parameter strings (which can be unique and numerous, such as UUIDs or user IDs) will persist in memory indefinitely, causing a slow memory leak.
Using Go 1.21's built-in clear() function before slicing to [:0] sets all elements to their zero value (""), allowing the matched parameter strings to be garbage collected.
| request.paramValues = request.paramValues[:0] | |
| clear(request.paramValues) | |
| request.paramValues = request.paramValues[:0] |
| // Errors flow through the tracking writer after the stdlib | ||
| // middleware wrapper returns. Use ErrorObserver for | ||
| // programmatic error observation. | ||
| if observedStatus != http.StatusOK { | ||
| t.Fatalf("expected stdlib middleware to see initial OK status, got %d", observedStatus) | ||
| } |
There was a problem hiding this comment.
Update Test Assertion to Verify Correct Error Observation
With the fix applied to UseHTTP to write the error response inside the standard middleware's execution context, the adapted middleware will now correctly observe the actual error status code (418 StatusTeapot) instead of the initial unwritten status (200 OK).
Update this assertion to verify that standard middleware can successfully observe the correct error status code.
// Standard middleware should correctly observe the written error status code.
if observedStatus != http.StatusTeapot {
t.Fatalf("expected stdlib middleware to observe status %d, got %d", http.StatusTeapot, observedStatus)
}| trimmed := strings.Trim(pattern, "/") | ||
| canonical := "/" + trimmed | ||
| trailing := canonical + "/" | ||
| if !strings.HasSuffix(pattern, "/") { | ||
| byPath[canonical] = index | ||
| if _, exists := byPath[trailing]; !exists { | ||
| byPath[trailing] = index | ||
| } | ||
| } else { | ||
| byPath[trailing] = index | ||
| if _, exists := byPath[canonical]; !exists { | ||
| byPath[canonical] = index | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid Registering Invalid "//" Static Route for Root Path
When pattern is "/", trimmed becomes "", canonical becomes "/", and trailing becomes "//". Under the current logic, this results in registering "//" as a static route in the byPath map:
byPath["//"] = indexTo prevent registering this invalid path, we should explicitly handle the root path case (trimmed == "") by registering only "/" and returning early.
trimmed := strings.Trim(pattern, "/")
canonical := "/" + trimmed
if trimmed == "" {
byPath["/"] = index
return
}
trailing := canonical + "/"
if !strings.HasSuffix(pattern, "/") {
byPath[canonical] = index
if _, exists := byPath[trailing]; !exists {
byPath[trailing] = index
}
} else {
byPath[trailing] = index
if _, exists := byPath[canonical]; !exists {
byPath[canonical] = index
}
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 874288b611
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| h := middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| handlerErr = next(req, res) |
There was a problem hiding this comment.
When standard middleware calls next.ServeHTTP with a wrapped writer or cloned request, this callback discards both w and r and invokes the Golpher handler with the originals. Consequently common middleware such as compression/status recorders cannot intercept downstream writes, and authentication or tracing context added with r.WithContext is invisible to the handler; the adapter must propagate these arguments into the pooled request/response path.
Useful? React with 👍 / 👎.
| func FromHTTPHandler(handler http.Handler) HandlerFunc { | ||
| return func(req *Request, res *Response) error { | ||
| return handler(req.acquireCtx(res), req, res) | ||
| handler.ServeHTTP(res.Raw(), req.http) |
There was a problem hiding this comment.
For routes mounted with FromHTTPHandler, a handler that reads r.Body directly receives the unwrapped stream because req.body() is never called. This bypasses both the new 1 MiB default and any configured MaxRequestBodyBytes or BodyLimit, allowing mounted handlers to consume arbitrarily large bodies despite the application's declared limit.
Useful? React with 👍 / 👎.
| for _, mw := range middlewares { | ||
| middleware := mw | ||
| app.Use(func(next HandlerFunc) HandlerFunc { |
There was a problem hiding this comment.
When UseHTTP receives multiple middleware values while the server is starting, each iteration calls Use under a separate lock. A first request can freeze the app after one iteration, causing the public UseHTTP call to panic on the next iteration while leaving only a prefix of the requested middleware installed; the entire variadic registration should be one locked mutation so it either completes before freeze or makes no change.
Useful? React with 👍 / 👎.
| if strings.HasPrefix(part, "*") { | ||
| name := part[1:] |
There was a problem hiding this comment.
The duplicate-name set is checked only for :param segments, so a pattern such as /files/:path/*path is accepted. Both captured values then share the same name, and Request.Param("path") always returns the first value, making the wildcard capture inaccessible; wildcard names should also be checked against and added to seenParams.
Useful? React with 👍 / 👎.
|
Emergency merge exception approved due to the confirmed GitHub Actions critical outage on 2026-07-20. Evidence before bypass:
The PR is mergeable without conflicts. It will be merged with a merge commit to preserve release/develop ancestry. Failed infrastructure checks must be rerun after GitHub Actions recovers. |
1 participant
Summary
Validation
Release