🔥 feat: Add StreamResponseBody support for the Client #3711
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds end-to-end response-body streaming: client API getters/setters, transport interface methods and implementations (standard/host/lb), Response.BodyStream()/IsStreaming(), Save() streaming branches, request BodyStream propagation, resource-safety fixes, extensive tests, and docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client
participant Transport
participant Core
participant fasthttp as fasthttp.Client
participant Response
User->>Client: SetStreamResponseBody(true)
Client->>Transport: SetStreamResponseBody(true)
Transport->>fasthttp: set StreamResponseBody flag
User->>Client: Do(request)
Client->>Core: executeRequest(...)
Core->>fasthttp: Do()/DoDeadline(...)
fasthttp-->>Core: *fasthttp.Response
alt request has BodyStream
Core->>fasthttp: SetBodyStream(contentLen, reader)
end
Core-->>Client: build Response (with RawResponse)
User->>Response: IsStreaming()
Response-->>User: true/false
alt streaming
User->>Response: BodyStream()
Response-->>User: io.Reader (stream)
User->>Response: Save(dest)
Response->>Response: io.Copy(BodyStream -> dest)
else non-streaming
User->>Response: Body()
Response-->>User: []byte
User->>Response: Save(dest)
Response->>Response: io.Copy(bytes.Reader -> dest)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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 @Abhirup-99, 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 introduces a significant enhancement to the client by adding support for streaming HTTP response bodies. This allows consumers to process large responses or server-sent events incrementally, preventing excessive memory consumption. The changes include new configuration options at both the client and individual request levels, along with the necessary logic to facilitate this streaming behavior and comprehensive test coverage.
Highlights
- Client-level Streaming Configuration: Introduced a streamResponseBody field and corresponding SetStreamResponseBody and StreamResponseBody methods to the Client struct, enabling global control over response body streaming.
- Request-level Streaming Override: Added a streamResponseBody field and methods to the Request struct, allowing per-request configuration that can override the client's default streaming setting.
- Dynamic Streaming Execution: Modified the core request execution logic to dynamically set and restore the underlying HTTP client's streaming option based on the active request or client configuration.
- Streamed Body Access: Implemented a BodyStream() method on the Response struct, providing an io.Reader interface for efficient, stream-based consumption of response bodies.
- Comprehensive Test Coverage: Added extensive unit tests to validate the new streaming functionality, covering scenarios like server-sent events, large responses, default settings, overrides, and method chaining.
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. ↩
|
I will update the documentation if the approach seems sound enough. |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for streaming HTTP response bodies in the client library, enabling responses to be read as streams instead of being fully loaded into memory. This is particularly useful for handling large responses or server-sent events.
- Added streaming configuration at both client and request levels with override capability
- Implemented
BodyStream()method on Response struct to provide streaming access - Added comprehensive test coverage for various streaming scenarios
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| client/client.go | Adds streamResponseBody field and configuration methods to Client struct |
| client/request.go | Adds per-request streaming configuration with client-level fallback |
| client/core.go | Updates request execution to handle streaming option setting and restoration |
| client/response.go | Implements BodyStream method for streaming response body access |
| client/client_test.go | Comprehensive tests for client-level streaming configuration and behavior |
| client/response_test.go | Tests for response streaming functionality including large data handling |
client/core.go
Outdated
| // Set StreamResponseBody option from request setting (falls back to client setting) | ||
| originalStreamResponseBody := c.client.fasthttp.StreamResponseBody | ||
| c.client.fasthttp.StreamResponseBody = c.req.StreamResponseBody() | ||
|
|
||
| var err error | ||
| go func() { | ||
| respv := fasthttp.AcquireResponse() | ||
| defer func() { | ||
| // Restore original StreamResponseBody setting | ||
| c.client.fasthttp.StreamResponseBody = originalStreamResponseBody |
There was a problem hiding this comment.
The StreamResponseBody setting is being modified on the shared client instance without proper synchronization. In a multi-threaded environment, concurrent requests could interfere with each other's streaming configuration. Consider using per-request client instances or adding proper locking mechanisms.
| // Set StreamResponseBody option from request setting (falls back to client setting) | |
| originalStreamResponseBody := c.client.fasthttp.StreamResponseBody | |
| c.client.fasthttp.StreamResponseBody = c.req.StreamResponseBody() | |
| var err error | |
| go func() { | |
| respv := fasthttp.AcquireResponse() | |
| defer func() { | |
| // Restore original StreamResponseBody setting | |
| c.client.fasthttp.StreamResponseBody = originalStreamResponseBody | |
| // Create a shallow copy of the fasthttp.Client to avoid data races on StreamResponseBody | |
| clientCopy := *c.client.fasthttp | |
| clientCopy.StreamResponseBody = c.req.StreamResponseBody() | |
| var err error | |
| go func() { | |
| respv := fasthttp.AcquireResponse() | |
| defer func() { |
client/core.go
Outdated
| // Set StreamResponseBody option from request setting (falls back to client setting) | ||
| originalStreamResponseBody := c.client.fasthttp.StreamResponseBody | ||
| c.client.fasthttp.StreamResponseBody = c.req.StreamResponseBody() | ||
|
|
||
| var err error | ||
| go func() { | ||
| respv := fasthttp.AcquireResponse() | ||
| defer func() { | ||
| // Restore original StreamResponseBody setting | ||
| c.client.fasthttp.StreamResponseBody = originalStreamResponseBody |
There was a problem hiding this comment.
The restoration of the original StreamResponseBody setting in the defer function may not execute in case of panics or early returns, potentially leaving the client in an inconsistent state. Additionally, this restoration happens in a goroutine which could create race conditions with other concurrent requests.
| // Set StreamResponseBody option from request setting (falls back to client setting) | |
| originalStreamResponseBody := c.client.fasthttp.StreamResponseBody | |
| c.client.fasthttp.StreamResponseBody = c.req.StreamResponseBody() | |
| var err error | |
| go func() { | |
| respv := fasthttp.AcquireResponse() | |
| defer func() { | |
| // Restore original StreamResponseBody setting | |
| c.client.fasthttp.StreamResponseBody = originalStreamResponseBody | |
| // Use a per-request copy of the fasthttp.Client to avoid data races on StreamResponseBody | |
| clientCopy := *c.client.fasthttp | |
| clientCopy.StreamResponseBody = c.req.StreamResponseBody() | |
| var err error | |
| go func() { | |
| respv := fasthttp.AcquireResponse() | |
| defer func() { |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
gaby
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
client/request.go (1)
594-605: Effective value resolution is correct and robustFalls back to the client setting when unset; default false if no client is attached yet. Consider documenting this precedence in the method comment for discoverability.
client/client_test.go (3)
1750-1767: Optional: make SSE handler actually stream (flush between events) for stronger signalRight now the handler writes three messages and returns. This can still pass even if the server buffers and sends a single chunk. Consider using SetBodyStreamWriter and flushing between writes to exercise true streaming behavior. Example:
app.Get("/sse", func(c fiber.Ctx) error { c.Set("Content-Type", "text/event-stream") c.Set("Cache-Control", "no-cache") c.Set("Connection", "keep-alive") c.Context().SetBodyStreamWriter(func(w *bufio.Writer) { msgs := []string{"data: message 1\n\n", "data: message 2\n\n", "data: message 3\n\n"} for _, m := range msgs { _, _ = w.WriteString(m) _ = w.Flush() time.Sleep(10 * time.Millisecond) } }) return nil })This reduces false positives where streaming isn’t actually occurring.
1836-1841: Strengthen fallback contract when streaming is disabled by reading BodyStreamYou assert BodyStream() is non-nil, but don’t verify it yields the same content as Body() in the fallback path. Read it and compare.
Apply this diff:
body := resp.Body() require.Equal(t, "Hello, World!", string(body)) bodyStream := resp.BodyStream() require.NotNil(t, bodyStream) + streamed, err := io.ReadAll(bodyStream) + require.NoError(t, err) + require.Equal(t, body, streamed)
1854-1883: Please add a follow-up request and Reset() behavior test to guard against per-request overrides leakingYou’ve already verified the getter/inheritance logic for StreamResponseBody, but we’re still missing two important checks in client/client_test.go around lines 1854–1883:
- A second request on the same client after an override, to ensure the per-request override doesn’t persist.
- Exercising
Request.Reset()to confirm it clears any per-request streaming flag and falls back to the client default.Suggested diff to insert after the existing assertions:
req2 := client.R().SetStreamResponseBody(false) require.False(t, req2.StreamResponseBody()) + // Follow-up call: ensure the override didn’t leak into actual behavior. + resp2b, err := req2.Get("http://" + addr + "/test") + require.NoError(t, err) + defer resp2b.Close() + require.Equal(t, "Hello, World!", string(resp2b.Body())) clientWithStreaming := New().SetStreamResponseBody(true) req3 := clientWithStreaming.R() require.True(t, req3.StreamResponseBody()) // Should inherit from client req4 := client.R(). SetStreamResponseBody(true). SetTimeout(time.Second * 5). SetStreamResponseBody(false) require.False(t, req4.StreamResponseBody()) + + // After Reset, per-request override should be cleared and revert to client setting (false). + reqReset := client.R().SetStreamResponseBody(true) + reqReset.Reset() + require.False(t, reqReset.StreamResponseBody())This will catch regressions where the underlying fasthttp flag isn’t properly restored and confirm Reset() clears the override.
📜 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 sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
client/client.go(2 hunks)client/client_test.go(1 hunks)client/core.go(1 hunks)client/request.go(3 hunks)client/response.go(1 hunks)client/response_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/response_test.go (1)
client/client.go (2)
Get(706-708)New(647-652)
client/client_test.go (1)
client/client.go (2)
New(647-652)Get(706-708)
🔇 Additional comments (6)
client/request.go (3)
70-72: Good choice: tri-state per-request overrideUsing a pointer to bool for streamResponseBody cleanly distinguishes unset from explicit true/false. LGTM.
606-612: Chainable setter is fineNo issues.
679-679: Reset clears the override as expectedResetting streamResponseBody to nil ensures subsequent requests inherit the client default. LGTM.
client/client_test.go (3)
1737-1745: LGTM: basic client-level toggle coverage is solidCovers default false, enable, disable. Clear, fast, and deterministic.
1793-1819: LGTM: large-response streaming vs non-streaming parityGood end-to-end validation that BodyStream() delivers identical data to Body() for large payloads, with streaming enabled/disabled. Using 1 MiB is a reasonable balance for CI.
1846-1852: LGTM: chainable method precedenceConfirms last-set-wins across chained setters with unrelated setters in between. Clear and sufficient.
client/client_test.go
Outdated
| buffer := make([]byte, 1024) | ||
| n, err := bodyStream.Read(buffer) | ||
| require.NoError(t, err) | ||
| require.Greater(t, n, 0) | ||
|
|
||
| content := string(buffer[:n]) | ||
| require.Contains(t, content, "data: message 1") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent flaky read assertion: allow io.EOF when n > 0
io.Reader is allowed to return (n > 0, err == io.EOF). Requiring NoError can intermittently fail. Accept EOF if bytes were read.
Apply this diff:
- buffer := make([]byte, 1024)
- n, err := bodyStream.Read(buffer)
- require.NoError(t, err)
- require.Greater(t, n, 0)
+ buffer := make([]byte, 1024)
+ n, err := bodyStream.Read(buffer)
+ require.Greater(t, n, 0)
+ if err != nil {
+ require.ErrorIs(t, err, io.EOF)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| buffer := make([]byte, 1024) | |
| n, err := bodyStream.Read(buffer) | |
| require.NoError(t, err) | |
| require.Greater(t, n, 0) | |
| content := string(buffer[:n]) | |
| require.Contains(t, content, "data: message 1") | |
| } | |
| buffer := make([]byte, 1024) | |
| n, err := bodyStream.Read(buffer) | |
| require.Greater(t, n, 0) | |
| if err != nil { | |
| require.ErrorIs(t, err, io.EOF) | |
| } | |
| content := string(buffer[:n]) | |
| require.Contains(t, content, "data: message 1") | |
| } |
🤖 Prompt for AI Agents
In client/client_test.go around lines 1781-1788, the test currently requires
NoError on bodyStream.Read which is flaky because io.Reader may return (n>0,
err==io.EOF); change the assertion to allow io.EOF when bytes were read: after
reading and asserting n>0, assert either err==nil or errors.Is(err, io.EOF) (or
use require.NoError when err==nil else require.True(errors.Is(err, io.EOF))),
and add imports for "io" and/or "errors" if not already present.
|
@Abhirup-99 The tests are failing. You can run them locally by running |
Abhirup-99
force-pushed
the
add-response
branch
from
a725b97 to
9c12637
Compare
|
@gaby fixed the failing tests |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3711 +/- ##
==========================================
+ Coverage 91.66% 91.67% +0.01%
==========================================
Files 119 119
Lines 10096 10144 +48
==========================================
+ Hits 9254 9300 +46
- Misses 534 537 +3
+ Partials 308 307 -1
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:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
client/client.go (2)
538-557: Reset should also clear streamResponseBody to default falseClient.Reset clears most flags but leaves streamResponseBody unchanged, leading to surprising behavior post-reset.
func (c *Client) Reset() { c.fasthttp = &fasthttp.Client{} c.baseURL = "" c.timeout = 0 c.userAgent = "" c.referer = "" c.retryConfig = nil c.debug = false + c.streamResponseBody = false
58-61: Guard temporary per-request flips of fasthttp.Client.StreamResponseBody with a dedicated mutex on ClientIf core executes by toggling the shared fasthttp.Client.StreamResponseBody between requests, concurrent requests can race. Add a small private mutex dedicated to this toggle. This keeps the public API unchanged and avoids overloading c.mu, which guards other state.
Apply:
type Client struct { logger log.CommonLogger fasthttp *fasthttp.Client @@ - timeout time.Duration - mu sync.RWMutex - debug bool - streamResponseBody bool + timeout time.Duration + mu sync.RWMutex + debug bool + streamResponseBody bool + // protects temporary flips of fasthttp.Client.StreamResponseBody during per-request execution + streamRespMu sync.Mutex }To confirm whether the toggle approach is used (vs. cloning a temp fasthttp.Client), run:
#!/bin/bash # Inspect how StreamResponseBody is applied during execution. rg -n -C2 --type=go 'StreamResponseBody' client/core.go client | sed -n '1,200p'client/response_test.go (1)
598-611: Fix flakiness: copy Body() before closing resp1normalBody aliases resp1’s internal buffer; after Close(), pooled buffers may be reused, causing nondeterministic failures. Copy the bytes before closing.
- defer resp1.Close() - normalBody := resp1.Body() + defer resp1.Close() + normalBody := append([]byte(nil), resp1.Body()...)
🧹 Nitpick comments (2)
client/client.go (2)
439-451: API shape LGTM; document precedence explicitly in the commentThe getter/setter are clear and chainable. Add one line noting that a request-level setting overrides the client-level default for discoverability.
// SetStreamResponseBody enables or disables response body streaming. // When enabled, the response body can be read as a stream using BodyStream() // instead of being fully loaded into memory. This is useful for large responses -// or server-sent events. +// or server-sent events. Note: a per-request setting (Request.SetStreamResponseBody) +// overrides this client-level default. func (c *Client) SetStreamResponseBody(enable bool) *Client { c.streamResponseBody = enable return c }
563-575: Optional: plumb StreamResponseBody through Config for convenienceMany helpers accept Config; adding an optional pointer makes it easy to set streaming per call without constructing a Request manually.
type Config struct { Ctx context.Context //nolint:containedctx // It's needed to be stored in the config. Body any Header map[string]string Param map[string]string Cookie map[string]string PathParam map[string]string FormData map[string]string UserAgent string Referer string File []*File Timeout time.Duration MaxRedirects int + // Optional per-request override; when set, overrides the client default. + StreamResponseBody *bool } @@ if cfg.MaxRedirects != 0 { req.SetMaxRedirects(cfg.MaxRedirects) } + if cfg.StreamResponseBody != nil { + req.SetStreamResponseBody(*cfg.StreamResponseBody) + } + if cfg.Body != nil { req.SetJSON(cfg.Body) return }Also applies to: 577-635
📜 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 sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
client/client.go(2 hunks)client/client_test.go(1 hunks)client/core.go(2 hunks)client/request.go(3 hunks)client/response.go(1 hunks)client/response_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- client/response.go
- client/client_test.go
- client/core.go
🔇 Additional comments (5)
client/response_test.go (1)
542-596: Solid coverage for basic and large streaming pathsThe test server and the two subtests validate BodyStream() on small and larger responses. Clear and deterministic.
client/request.go (4)
70-72: *Good choice using bool to represent tri-state (unset vs true/false)This enables request-level override while preserving client default. No action needed.
594-605: Precedence and fallback logic reads correctlyReturns request override when set; otherwise falls back to client-level; defaults to false if no client. Matches the intended design.
606-612: Setter is fine; pointer escape is safeUsing &enable is idiomatic (escapes to heap). Chainable API is consistent with others.
679-679: Reset correctly clears the per-request overrideResetting streamResponseBody to nil ensures the request returns to default behavior. Good.
Abhirup-99
force-pushed
the
add-response
branch
from
9c12637 to
4d32c17
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/core.go (1)
132-141: Minor duplication: unify Do vs DoRedirects callsitesThe four callsites using
fastHTTPClient.Do/DoRedirectsunder retry and non-retry paths duplicate logic. A tiny helper makes it harder to regress:- if cfg != nil { - err = retry.NewExponentialBackoff(*cfg).Retry(func() error { - if shouldRedirect { - return fastHTTPClient.DoRedirects(reqv, respv, c.req.maxRedirects) - } - return fastHTTPClient.Do(reqv, respv) - }) - } else { - if shouldRedirect { - err = fastHTTPClient.DoRedirects(reqv, respv, c.req.maxRedirects) - } else { - err = fastHTTPClient.Do(reqv, respv) - } - } +call := func() error { + if shouldRedirect { + return fastHTTPClient.DoRedirects(reqv, respv, c.req.maxRedirects) + } + return fastHTTPClient.Do(reqv, respv) +} +if cfg != nil { + err = retry.NewExponentialBackoff(*cfg).Retry(call) +} else { + err = call() +}Low impact, but improves readability.
📜 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 sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
client/client.go(2 hunks)client/client_test.go(1 hunks)client/core.go(2 hunks)client/request.go(3 hunks)client/response.go(1 hunks)client/response_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- client/response.go
- client/client.go
- client/request.go
- client/response_test.go
- client/client_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/core.go (1)
client/client.go (1)
Client(32-62)
🔇 Additional comments (3)
client/core.go (3)
84-118: Kudos: avoids flipping shared fasthttp.Client.StreamResponseBodyNot mutating the shared client’s flag sidesteps the prior data race concerns. Good move. If you adopt the dual-client approach suggested above, you’ll keep this benefit while restoring pooling.
149-151: No action needed: CopyTo + ReleaseResponse ordering is safeVerified that fasthttp.Response.CopyTo performs a synchronous copy of the body into resp.RawResponse (rather than merely reassigning a stream pointer), and once CopyTo returns, you can immediately call ReleaseResponse(respv) without affecting the destination’s BodyStream. The existing code at client/core.go (lines 149–151) is correct and requires no changes.
88-118: Verify comprehensive cloning offasthttp.Clientfields
I wasn’t able to locateclient/core.goin the repository, but manually copying each exported field fromfasthttp.Clientis brittle and prone to omissions (e.g., new fields likeMaxConnWaitTimeout,RetryIf, etc., won’t be copied and can silently change behavior). Please verify that you:
- Mirror all exported fields when creating a temporary client for per-request overrides.
- Centralize this logic in a helper (e.g., reflection-based) to automatically pick up new fields.
- Add a unit test that fails if any exported field in
fasthttp.Clientisn’t present in the clone.- Or consider toggling
StreamResponseBodyon the shared client under a lock instead of cloning.Let me know if you’d like a PR-ready helper and reflection-based test snippet for this.
Abhirup-99
force-pushed
the
add-response
branch
from
4d32c17 to
28342fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
client/response.go (2)
92-101: Tighten BodyStream docs: lifetime, connection reuse, and Body() emptiness when streamingThe code is fine. Please clarify the contract and call order to prevent misuse with pooled responses and fasthttp’s connection reuse.
Apply this doc tweak:
-// BodyStream returns the response body as a stream reader. -// Note: When using BodyStream(), the response body is not copied to memory, -// so calling Body() afterwards may return an empty slice. +// BodyStream returns an io.Reader for the response body. +// If streaming was enabled (via Request.SetStreamResponseBody, Client.SetStreamResponseBody, +// or the underlying fasthttp.Client.StreamResponseBody), the reader yields bytes directly from +// the connection without buffering in memory. +// +// Important: +// - Consume the reader before calling Response.Close(). +// - Read the stream to EOF to allow connection reuse by the underlying client. +// - When streaming is enabled, Body() (and helpers like String/JSON/XML/CBOR/Save) may be empty; +// prefer io.Copy with BodyStream() for large responses. +// If streaming is not enabled, BodyStream returns a bytes.Reader over Body(). func (r *Response) BodyStream() io.Reader {
150-165: Save() ignores streaming and closes caller’s writer — stream from BodyStream() and don’t close external writers
- Using Body() makes Save() write an empty file/output when streaming is enabled.
- Closing an io.Writer that the function didn’t open is surprising and can break callers.
Apply this minimal change:
- if _, err = io.Copy(outFile, bytes.NewReader(r.Body())); err != nil { + if _, err = io.Copy(outFile, r.BodyStream()); err != nil { return fmt.Errorf("failed to write response body to file: %w", err) } @@ - case io.Writer: - if _, err := io.Copy(p, bytes.NewReader(r.Body())); err != nil { + case io.Writer: + if _, err := io.Copy(p, r.BodyStream()); err != nil { return fmt.Errorf("failed to write response body to io.Writer: %w", err) } - defer func() { - if pc, ok := p.(io.WriteCloser); ok { - _ = pc.Close() //nolint:errcheck // not needed - } - }() return nilFollow-up (optional): consider adding a short note in Save()’s doc that it respects streaming and doesn’t close external writers.
client/response_test.go (1)
606-629: Avoid retaining Body()’s internal buffer across Close; also assert Body() is empty when streamingTo prevent future flakiness with pooled buffers and to codify the streaming contract, copy the non-streamed body and add an assertion that Body() is empty when streaming is enabled.
Apply:
- defer resp1.Close() - normalBody := resp1.Body() + defer resp1.Close() + normalBody := append([]byte(nil), resp1.Body()...) // copy before pool reuse @@ - streamedBody, err := io.ReadAll(resp2.BodyStream()) + streamedBody, err := io.ReadAll(resp2.BodyStream()) require.NoError(t, err) require.Equal(t, normalBody, streamedBody) + // streaming mode shouldn't buffer Body() + require.Len(t, resp2.Body(), 0)
🧹 Nitpick comments (1)
client/response_test.go (1)
542-629: Add a streaming Save() regression test (uses BodyStream and does not close external writer)Given the Save() fix, add coverage to ensure we don’t regress: the writer must not be closed by Save(), and content must be streamed correctly when streaming is enabled.
Insert this new subtest at the end of Test_Response_BodyStream (before the final closing brace):
+ t.Run("Save streams and does not close external writer", func(t *testing.T) { + t.Parallel() + + server := startTestServer(t, func(app *fiber.App) { + app.Get("/stream", func(c fiber.Ctx) error { + return c.SendString("streaming data") + }) + }) + defer server.stop() + + client := New().SetDial(server.dial()).SetStreamResponseBody(true) + resp, err := client.Get("http://example.com/stream") + require.NoError(t, err) + defer resp.Close() + + type trackingBuf struct { + bytes.Buffer + closed bool + } + // implements io.WriteCloser to verify Save() does not close it + var w = &trackingBuf{} + // wrap with NopCloser-like shim + writer := struct { + io.Writer + io.Closer + }{ + Writer: w, + Closer: io.CloserFunc(func() error { + w.closed = true + return nil + }), + } + + // Save to writer; it must not close it + err = resp.Save(writer) + require.NoError(t, err) + require.Equal(t, "streaming data", w.String()) + require.False(t, w.closed, "Save must not close external writers") + })If you prefer, we can place a similar subtest under Test_Response_Save.
📜 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 sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
client/client.go(2 hunks)client/client_test.go(1 hunks)client/core.go(2 hunks)client/request.go(3 hunks)client/response.go(1 hunks)client/response_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- client/client.go
- client/request.go
- client/core.go
- client/client_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/response_test.go (1)
client/client.go (2)
Get(706-708)New(647-652)
Abhirup-99
force-pushed
the
add-response
branch
from
28342fa to
ddbb831
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
client/response.go (2)
92-101: Tighten BodyStream contract; clarify lifetime and fallback behaviorDoc is still a bit vague about ordering/lifetime and the fallback to an in-memory reader. Propose expanding the comment to state the contract explicitly and reduce surprises.
-// BodyStream returns the response body as a stream reader. -// Note: When using BodyStream(), the response body is not copied to memory, -// so calling Body() afterwards may return an empty slice. +// BodyStream returns an io.Reader for the response body. +// +// If streaming was enabled (via Request.SetStreamResponseBody, Client.SetStreamResponseBody, +// or the underlying fasthttp.Client.StreamResponseBody), the reader yields data directly from +// the connection without buffering. Otherwise, it returns a bytes.Reader over Body(). +// +// Important: +// - Consume the reader before calling Response.Close(). +// - When streaming is enabled, Body() (and helpers like String/JSON/XML/CBOR/Save) may observe +// an empty body. Prefer io.Copy with BodyStream() for large responses. func (r *Response) BodyStream() io.Reader { if stream := r.RawResponse.BodyStream(); stream != nil { return stream } // If streaming is not enabled, return a bytes.Reader from the regular body return bytes.NewReader(r.RawResponse.Body()) }
161-172: Make Save() always stream via BodyStream(); also stream when saving to a fileThe writer branch partially streams, but the file path branch still buffers via Body(). Since BodyStream() already falls back to a bytes.Reader when streaming isn’t enabled, you can unconditionally copy from BodyStream() in both branches. This avoids large in-memory buffers and simplifies the code.
@@ - if _, err = io.Copy(outFile, bytes.NewReader(r.Body())); err != nil { + if _, err = io.Copy(outFile, r.BodyStream()); err != nil { return fmt.Errorf("failed to write response body to file: %w", err) } @@ - case io.Writer: - var err error - if r.IsStreaming() { - _, err = io.Copy(p, r.BodyStream()) - } else { - _, err = io.Copy(p, bytes.NewReader(r.Body())) - } - - if err != nil { - return fmt.Errorf("failed to write response body to writer: %w", err) - } - - return nil + case io.Writer: + if _, err := io.Copy(p, r.BodyStream()); err != nil { + return fmt.Errorf("failed to write response body to writer: %w", err) + } + return nilFollow-up: consider adding/adjusting tests to cover Save(filePath) with streaming enabled (e.g., large response) to ensure we don’t regress. I can draft that if helpful.
client/core.go (1)
84-120: Per-request allocating a new fasthttp.Client loses connection pooling and adds substantial latency/TLS overheadConstructing a fresh
*fasthttp.Clientwhenever the per-request streaming flag differs discards the shared connection pool and TLS session cache, causing extra TCP/TLS handshakes and reduced throughput under mixed workloads. This will regress performance for SSE/large downloads that toggle streaming on/off across requests.Consider maintaining two long-lived internal clients (streaming vs non-streaming) on
Clientand routing each request accordingly. Here's a suggested implementation:Add to
client/client.go:+// In Client struct: +streamingFHC *fasthttp.Client // lazily initialized clone with StreamResponseBody=true + +// pickFastHTTPClient returns a pooled client configured for the requested streaming mode. +func (c *Client) pickFastHTTPClient(stream bool) *fasthttp.Client { + c.mu.RLock() + defer c.mu.RUnlock() + + if stream == c.streamResponseBody { + return c.fasthttp + } + // Lazily build the streaming variant once and reuse it. + if stream && c.streamingFHC != nil { + return c.streamingFHC + } + return c.initStreamingClientLocked(stream) +} + +// initStreamingClientLocked clones exported config into a new fasthttp.Client. +// c.mu must be held by the caller. +func (c *Client) initStreamingClientLocked(stream bool) *fasthttp.Client { + o := c.fasthttp + clone := &fasthttp.Client{ + Dial: o.Dial, + DialDualStack: o.DialDualStack, + TLSConfig: o.TLSConfig, + MaxConnsPerHost: o.MaxConnsPerHost, + MaxIdleConnDuration: o.MaxIdleConnDuration, + MaxConnDuration: o.MaxConnDuration, + ReadTimeout: o.ReadTimeout, + WriteTimeout: o.WriteTimeout, + ReadBufferSize: o.ReadBufferSize, + WriteBufferSize: o.WriteBufferSize, + MaxResponseBodySize: o.MaxResponseBodySize, + NoDefaultUserAgentHeader: o.NoDefaultUserAgentHeader, + DisableHeaderNamesNormalizing: o.DisableHeaderNamesNormalizing, + DisablePathNormalizing: o.DisablePathNormalizing, + MaxIdemponentCallAttempts: o.MaxIdemponentCallAttempts, + Name: o.Name, + ConfigureClient: o.ConfigureClient, + StreamResponseBody: stream, + } + if stream { + c.streamingFHC = clone + } + return clone +}Then simplify this code block:
- // Determine which client to use - create a new one if StreamResponseBody differs - var fastHTTPClient *fasthttp.Client - requestStreamResponseBody := c.req.StreamResponseBody() - c.client.mu.RLock() - clientStream := c.client.streamResponseBody - original := c.client.fasthttp - - if requestStreamResponseBody != clientStream { - // Request setting differs from client setting, create a temporary client - - fastHTTPClient = &fasthttp.Client{ - Dial: original.Dial, - DialDualStack: original.DialDualStack, - TLSConfig: original.TLSConfig, - MaxConnsPerHost: original.MaxConnsPerHost, - MaxIdleConnDuration: original.MaxIdleConnDuration, - MaxConnDuration: original.MaxConnDuration, - ReadTimeout: original.ReadTimeout, - WriteTimeout: original.WriteTimeout, - ReadBufferSize: original.ReadBufferSize, - WriteBufferSize: original.WriteBufferSize, - MaxResponseBodySize: original.MaxResponseBodySize, - NoDefaultUserAgentHeader: original.NoDefaultUserAgentHeader, - DisableHeaderNamesNormalizing: original.DisableHeaderNamesNormalizing, - DisablePathNormalizing: original.DisablePathNormalizing, - MaxIdemponentCallAttempts: original.MaxIdemponentCallAttempts, - Name: original.Name, - ConfigureClient: original.ConfigureClient, - - // Request-specific override - StreamResponseBody: requestStreamResponseBody, - } - } else { - fastHTTPClient = original - } - c.client.mu.RUnlock() + // Determine which client to use without sacrificing connection pooling + requestStreamResponseBody := c.req.StreamResponseBody() + fastHTTPClient := c.client.pickFastHTTPClient(requestStreamResponseBody)This keeps pooling intact and avoids repeated allocation while maintaining thread safety.
🧹 Nitpick comments (1)
client/response.go (1)
103-106: IsStreaming semantics are OK; consider documenting that it reflects fasthttp.BodyStream presenceMinor: IsStreaming simply mirrors RawResponse.BodyStream() != nil. A short note in the comment that this reports whether the underlying fasthttp response is streaming (not whether data remains unread) would avoid confusion.
📜 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 sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
client/client.go(2 hunks)client/client_test.go(1 hunks)client/core.go(2 hunks)client/request.go(3 hunks)client/response.go(2 hunks)client/response_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- client/response_test.go
- client/client_test.go
- client/client.go
- client/request.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/core.go (1)
client/client.go (1)
Client(32-62)
🔇 Additional comments (1)
client/core.go (1)
133-142: LGTM!The routing to use
fastHTTPClientinstead of the hardcoded client is correct and properly handles both retry and non-retry paths with redirects.
|
@Abhirup-99 Can you take a look at the coverage report from the codecov workflow? I think a few of the low coverage lines can be covered with a few simple unit tests. |
Abhirup-99
force-pushed
the
add-response
branch
from
6751f82 to
c4c1dc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/response.go (1)
155-176: Drop the duplicated branches in Save and rely on BodyStream’s fallback
BodyStream()already returns the raw fasthttp stream when streaming is enabled and a fresh*bytes.Readerwhen it isn’t, so you can skip theIsStreaming()checks and call it unconditionally. That trims duplicate code and avoids the secondRawResponse.Body()call.- if r.IsStreaming() { - _, err = io.Copy(outFile, r.BodyStream()) - } else { - _, err = io.Copy(outFile, bytes.NewReader(r.Body())) - } + _, err = io.Copy(outFile, r.BodyStream()) @@ - var err error - if r.IsStreaming() { - _, err = io.Copy(p, r.BodyStream()) - } else { - _, err = io.Copy(p, bytes.NewReader(r.Body())) - } + var err error + _, err = io.Copy(p, r.BodyStream())client/transport_test.go (2)
414-421: Extend test to verify enable/disable with empty clients.The "empty clients" subtest only checks the default value. Consider adding assertions to verify that calling
SetStreamResponseBody(true)andSetStreamResponseBody(false)with an empty client list doesn't panic and returns the expected value.Apply this diff to make the test more complete:
t.Run("empty clients", func(t *testing.T) { t.Parallel() lbClient := &fasthttp.LBClient{ Clients: []fasthttp.BalancingClient{}, } transport := newLBClientTransport(lbClient) require.False(t, transport.StreamResponseBody()) + + // Verify enable/disable doesn't panic with empty clients + transport.SetStreamResponseBody(true) + require.True(t, transport.StreamResponseBody()) + transport.SetStreamResponseBody(false) + require.False(t, transport.StreamResponseBody()) })
467-488: Add assertions for transport return value for consistency.Unlike other subtests, the "nested lb client" test doesn't verify
transport.StreamResponseBody()return value after callingSetStreamResponseBody. For consistency with the other subtests (e.g., "single host client", "multiple host clients"), add assertions to check the transport's return value.Apply this diff to improve consistency:
t.Run("nested lb client", func(t *testing.T) { t.Parallel() hostClient1 := &fasthttp.HostClient{Addr: "example1.com:80"} hostClient2 := &fasthttp.HostClient{Addr: "example2.com:80"} nestedLB := &fasthttp.LBClient{ Clients: []fasthttp.BalancingClient{hostClient1, hostClient2}, } lbClient := &fasthttp.LBClient{ Clients: []fasthttp.BalancingClient{&lbBalancingClient{client: nestedLB}}, } transport := newLBClientTransport(lbClient) // Enable streaming on nested clients transport.SetStreamResponseBody(true) + require.True(t, transport.StreamResponseBody()) require.True(t, hostClient1.StreamResponseBody) require.True(t, hostClient2.StreamResponseBody) // Disable streaming on nested clients transport.SetStreamResponseBody(false) + require.False(t, transport.StreamResponseBody()) require.False(t, hostClient1.StreamResponseBody) require.False(t, hostClient2.StreamResponseBody) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
client/client.go(1 hunks)client/client_test.go(1 hunks)client/core.go(2 hunks)client/core_test.go(1 hunks)client/response.go(3 hunks)client/response_test.go(2 hunks)client/transport.go(4 hunks)client/transport_test.go(1 hunks)ctx_test.go(2 hunks)docs/client/rest.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- ctx_test.go
- client/client.go
- client/core.go
- docs/client/rest.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
client/response.goclient/transport_test.goclient/client_test.goclient/core_test.goclient/transport.goclient/response_test.go
🧠 Learnings (9)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
client/response.goclient/client_test.goclient/response_test.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Applied to files:
client/response.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
client/response.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
client/client_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
client/client_test.goclient/response_test.go
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
client/client_test.goclient/response_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
client/client_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
client/response_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
client/response_test.go
🧬 Code graph analysis (2)
client/client_test.go (1)
client/client.go (3)
New(752-757)NewWithHostClient(768-773)NewWithLBClient(776-781)
client/response_test.go (3)
client/client.go (2)
Get(831-833)New(752-757)ctx_interface_gen.go (1)
Ctx(18-428)client/response.go (3)
Response(19-25)AcquireResponse(222-228)ReleaseResponse(232-235)
🔇 Additional comments (2)
client/transport_test.go (2)
350-378: LGTM! Comprehensive streaming toggle tests.The test thoroughly covers default state, enabling, and disabling streaming for StandardClientTransport, verifying both the transport wrapper and the underlying fasthttp.Client reflect the changes correctly.
380-409: LGTM! Consistent test coverage for HostClientTransport.The test structure mirrors the StandardClientTransport tests and correctly verifies streaming state propagation to the underlying HostClient.
Abhirup-99
force-pushed
the
add-response
branch
from
c4c1dc7 to
402be56
Compare
|
@grivera64 can you check now? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/response.go (1)
92-106: Consider clarifying BodyStream() documentation.The implementation is correct, but the documentation could be more explicit about the consumption contract and relationship with Body(). Consider expanding the doc to note:
- The reader must be consumed before closing the Response
- When streaming is enabled, Body() may return empty after consuming BodyStream()
- For non-streaming responses, BodyStream() returns a bytes.Reader over the buffered body
Example enhancement:
-// BodyStream returns the response body as a stream reader. -// Note: When using BodyStream(), the response body is not copied to memory, -// so calling Body() afterwards may return an empty slice. +// BodyStream returns an io.Reader for the response body. +// When streaming is enabled (via SetStreamResponseBody), the reader yields data +// directly from the connection without buffering. Otherwise, it returns a bytes.Reader +// over the buffered body. +// +// Important: consume the reader before calling Response.Close(). When streaming is +// enabled, Body() may return empty after the stream has been read. func (r *Response) BodyStream() io.Reader {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
client/client.go(1 hunks)client/client_test.go(1 hunks)client/core.go(2 hunks)client/core_test.go(1 hunks)client/response.go(3 hunks)client/response_test.go(2 hunks)client/transport.go(4 hunks)client/transport_test.go(1 hunks)ctx_test.go(2 hunks)docs/client/rest.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- client/client_test.go
- client/transport_test.go
- client/core_test.go
- docs/client/rest.md
- ctx_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
client/core.goclient/client.goclient/response_test.goclient/transport.goclient/response.go
🧠 Learnings (7)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
client/response_test.goclient/response.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
client/response_test.go
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
client/response_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
client/response_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
client/response_test.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Applied to files:
client/response.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
client/response.go
🧬 Code graph analysis (2)
client/core.go (1)
client/response.go (1)
ReleaseResponse(232-235)
client/response_test.go (2)
client/client.go (2)
Get(831-833)New(752-757)client/response.go (3)
Response(19-25)AcquireResponse(222-228)ReleaseResponse(232-235)
🔇 Additional comments (13)
client/client.go (1)
529-541: LGTM! Clean delegation to transport layer.The methods correctly delegate streaming configuration to the transport interface, which handles the underlying fasthttp client(s) appropriately. The implementation is thread-safe since the transport layer manages synchronization internally.
client/core.go (3)
89-93: LGTM! Nil check prevents double-free.The conditional release correctly guards against releasing a nil response, which can happen when respv is reassigned to nil after swapping into resp.RawResponse (line 127).
96-98: LGTM! BodyStream forwarding preserves streaming semantics.Correctly propagates the request's BodyStream to the fasthttp request, maintaining the original ContentLength. This enables streaming request bodies alongside the new streaming response body support.
125-130: LGTM! RawResponse swap prevents resource leaks.The swap sequence is correct:
- Save original RawResponse reference
- Assign the new respv to resp.RawResponse
- Nil out respv (preventing double-free in defer)
- Release the original RawResponse if it existed
This ensures clean resource management when responses are reused or when RawResponse is replaced.
client/response.go (1)
155-179: LGTM! Save() correctly handles both streaming and non-streaming paths.The conditional branching on
IsStreaming()ensures efficient streaming when enabled while maintaining backward compatibility for buffered responses. Usingio.Copyis appropriate for both cases.client/response_test.go (4)
546-654: LGTM! Comprehensive streaming Save() tests with proper error handling.The new tests correctly exercise:
- Streaming to io.Writer without closing (verifying Save doesn't close user-provided writers)
- Write errors during streaming (using the testErrorWriter helper)
The test helpers (testClosableBuffer, testErrorWriter) are well-designed for their purpose.
656-1083: Excellent streaming test coverage with true server-side streaming.The tests now properly use
SendStreamWriterandSendStreamas requested in past reviews, ensuring the server actually streams data progressively. Coverage includes:
- Basic streaming and large responses
- Chunked streaming with delays
- Server-sent events
- Progressive JSON streaming
- Connection interruption handling
- Large response validation
- Stream identity verification
All scenarios exercise real streaming behavior rather than masking it with client-side buffering.
1085-1244: LGTM! Thorough fallback and state consistency tests.The fallback tests verify:
- Non-streaming responses correctly use bytes.Reader fallback
- Empty responses are handled correctly
- Large non-streaming responses work via fallback path
- BodyStream() works regardless of streaming state
- IsStreaming() accurately reflects the streaming mode
This ensures the API is robust for both streaming and non-streaming use cases.
1246-1373: LGTM! Complete Save() streaming validation.Additional tests cover:
- File-path streaming (end-to-end)
- File write errors (permission/path issues)
- Acquire/Release pooling correctness
- Large streaming writes with error injection
The coverage is comprehensive and exercises real-world failure scenarios.
client/transport.go (4)
30-31: LGTM! Clean interface extension for streaming support.The new methods integrate naturally with the existing httpClientTransport interface, maintaining consistency with the getter/setter pattern used for TLSConfig and Dial.
80-86: LGTM! Straightforward delegation to fasthttp.Client.The implementation correctly proxies to the underlying fasthttp.Client's StreamResponseBody field for both getter and setter.
134-140: LGTM! Straightforward delegation to fasthttp.HostClient.The implementation correctly proxies to the underlying fasthttp.HostClient's StreamResponseBody field for both getter and setter.
200-221: LGTM! Correct traversal logic for nested load balancers.The implementation properly handles the LBClient's nested structure:
- Getter: Returns false if no clients exist (safe default), then walks the first client tree to find the StreamResponseBody setting
- Setter: Uses
forEachHostClientto propagate the setting to all nested HostClient instancesThis ensures consistent behavior across nested and flat load-balancing configurations.
Abhirup-99
force-pushed
the
add-response
branch
from
402be56 to
3c9c80c
Compare
|
fixed the lint errors |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ctx_test.go (1)
3778-3778: Verify timeout necessity for file save operations.The 5-second timeout (5x the default) added to this file save test suggests potential performance concerns or CI environment slowness. While reasonable for file I/O, ensure this isn't masking an underlying issue with file save performance.
Consider:
- Is this timeout consistently needed, or only in specific CI environments?
- Should other file operation tests (
Test_Ctx_FormFileat line 1806, variousTest_Ctx_SendFile*tests) also have explicit timeouts for consistency?docs/client/rest.md (1)
409-423: Consider enhancing streaming documentation with usage examples and caveats.The current documentation provides basic information but could benefit from additional context to help users avoid common pitfalls:
- Default behavior (streaming disabled by default)
- How to override at request level via
Request.SetStreamResponseBody(bool)- Usage caution: when streaming is enabled,
Body()may return empty; preferBodyStream()for consumption- A minimal example showing how to consume the stream (e.g.,
io.Copy(file, resp.BodyStream()))- Note about checking
Response.IsStreaming()to verify streaming stateclient/response.go (2)
92-101: Consider clarifying the documentation regarding Body() behavior when streaming.The note mentions that
Body()may return an empty slice after usingBodyStream(), but it could be more explicit about when this occurs (specifically, when the underlying fasthttp response is in streaming mode and the stream has been consumed).Consider this documentation refinement:
-// BodyStream returns the response body as a stream reader. -// Note: When using BodyStream(), the response body is not copied to memory, -// so calling Body() afterwards may return an empty slice. +// BodyStream returns an io.Reader for the response body. +// If streaming is enabled (via Client.SetStreamResponseBody or Request.SetStreamResponseBody), +// this returns the underlying stream reader without buffering. +// Otherwise, it returns a bytes.Reader wrapping the buffered body. +// +// Note: When streaming is enabled and the stream is consumed, Body() will return an empty slice. +// Check IsStreaming() to determine the current mode.
155-179: Consider consolidating the streaming/non-streaming branches in Save().Both the file path and io.Writer cases use identical branching logic (
if r.IsStreaming() ... else ...). SinceBodyStream()already handles both streaming and non-streaming cases by returning the appropriate reader, the conditional branches could be simplified.Apply this diff to simplify and reduce duplication:
// Create and write to file outFile, err := os.Create(file) if err != nil { return fmt.Errorf("failed to create file: %w", err) } defer func() { _ = outFile.Close() }() //nolint:errcheck // not needed - if r.IsStreaming() { - _, err = io.Copy(outFile, r.BodyStream()) - } else { - _, err = io.Copy(outFile, bytes.NewReader(r.Body())) - } + _, err = io.Copy(outFile, r.BodyStream()) if err != nil { return fmt.Errorf("failed to write response body to file: %w", err) } return nil case io.Writer: - var err error - if r.IsStreaming() { - _, err = io.Copy(p, r.BodyStream()) - } else { - _, err = io.Copy(p, bytes.NewReader(r.Body())) - } + _, err := io.Copy(p, r.BodyStream()) if err != nil { return fmt.Errorf("failed to write response body to writer: %w", err) } return nilThis simplification is safe because
BodyStream()already provides the correct reader for both modes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
client/client.go(1 hunks)client/client_test.go(1 hunks)client/core.go(2 hunks)client/core_test.go(1 hunks)client/response.go(3 hunks)client/response_test.go(2 hunks)client/transport.go(4 hunks)client/transport_test.go(1 hunks)ctx_test.go(2 hunks)docs/client/rest.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/client_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/core.go
- client/transport.go
- client/client.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)
Files:
client/core_test.goclient/response.goclient/response_test.goclient/transport_test.goctx_test.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/client/rest.md
🧠 Learnings (12)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
client/response.goclient/response_test.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Applied to files:
client/response.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
client/response.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
client/response_test.goctx_test.go
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
client/response_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
client/response_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
client/response_test.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
ctx_test.go
🧬 Code graph analysis (2)
client/response_test.go (2)
client/client.go (2)
Get(831-833)New(752-757)client/response.go (3)
Response(19-25)AcquireResponse(222-228)ReleaseResponse(232-235)
ctx_test.go (1)
app.go (1)
TestConfig(1090-1101)
🔇 Additional comments (10)
ctx_test.go (1)
3820-3820: Consistent timeout for memory storage test.This test uses in-memory storage yet requires the same 5-second timeout as the disk-based
Test_Ctx_SaveFile. This suggests the timeout is needed for multipart upload processing rather than the storage backend itself, which is reasonable.For consistency, verify that all file upload/save tests have appropriate timeouts to prevent test flakiness in CI environments.
client/core_test.go (1)
386-391: LGTM!The streaming method stubs correctly satisfy the transport interface for this test helper. The no-op implementation is appropriate since
blockingErrTransportis designed to simulate errors, not actual streaming behavior.client/response.go (1)
103-106: LGTM!The
IsStreaming()implementation correctly identifies when the response is in streaming mode by checking for a non-nilBodyStream().client/transport_test.go (1)
350-548: LGTM! Comprehensive streaming state test coverage.The tests thoroughly validate streaming state propagation across all transport implementations:
- StandardClientTransport: verifies default, enable, and disable behavior with fasthttp.Client state checks
- HostClientTransport: validates streaming toggles on HostClient
- LBClientTransport: covers empty, single, multiple, nested, and mixed client configurations
- Interface tests: ensure consistent behavior across all transport types
The nested LB client scenarios are particularly valuable for catching propagation bugs in complex load balancing setups.
client/response_test.go (6)
546-620: LGTM! Well-designed Save() streaming tests with proper error handling.The new streaming-specific Save() tests effectively validate:
- Writer closing behavior: correctly verifies that Save() does not close the provided writer (lines 546-585)
- Write error handling: uses a custom
testErrorWriterto simulate mid-stream write failures and verify proper error propagation (lines 587-619)Both tests use
SendStreamWriterto ensure genuine streaming behavior, addressing previous review feedback.
622-654: LGTM! Clean test helper implementations.The test helper types are well-designed for their specific purposes:
testClosableBuffer: tracks Close() calls to verify Save() doesn't close provided writerstestErrorWriter: simulates write failures after a threshold to test error handling in streaming pathsBoth helpers are appropriately scoped (non-exported) and implement only the necessary interfaces.
656-1083: Excellent comprehensive streaming test coverage.The
Test_Response_BodyStreamtest suite thoroughly validates streaming behavior across diverse scenarios:
- Basic streaming (lines 659-679): verifies simple stream consumption
- Large response streaming (lines 681-718): validates progressive reads with buffer management
- Compare with regular body (lines 720-751): ensures streaming and non-streaming produce identical results
- Chunked streaming with delays (lines 753-804): tests incremental data arrival
- Server-sent events (lines 806-858): validates SSE streaming pattern
- Progressive JSON streaming (lines 860-909): tests streaming JSON arrays
- Connection interruption (lines 911-972): validates behavior when stream is closed mid-read
- Large response validation (lines 974-1040): tests multi-chunk streaming with pattern verification
- Stream object identity (lines 1042-1082): verifies correct reader type based on streaming state
All subtests properly use
SendStreamWriterorSendStreamto ensure genuine streaming behavior, addressing past review concerns.
1085-1169: LGTM! Comprehensive fallback behavior tests.The
Test_Response_BodyStream_Fallbacksuite effectively validates non-streaming scenarios:
- Non-streaming fallback (lines 1087-1107): verifies bytes.Reader fallback when streaming is disabled
- Empty response (lines 1109-1129): tests edge case of empty body
- Large non-streaming response (lines 1131-1168): confirms fallback works correctly even with large payloads
These tests ensure
BodyStream()provides a consistent API regardless of streaming mode.
1171-1244: LGTM! IsStreaming() behavior properly validated.The
Test_Response_IsStreamingsuite covers:
- Streaming enabled with large response (lines 1174-1196)
- Streaming disabled (lines 1198-1212)
- BodyStream() works regardless of streaming state (lines 1214-1243)
The third subtest is particularly valuable as it confirms the API remains consistent whether streaming is enabled or not.
1246-1373: LGTM! Additional Save() test coverage for edge cases.The remaining Save() tests validate:
- Streaming to file path (lines 1246-1293): confirms Save() works correctly when streaming to a file path
- File error handling (lines 1295-1315): validates error handling when file creation fails
- Acquire/Release pool behavior (lines 1317-1338): verifies response pooling works correctly
- File write errors (lines 1340-1373): tests error handling when writing to
/dev/fullfailsThese tests provide good coverage of error paths and edge cases.
|
@grivera64 bump |
|
@codex review |
|
/gemini review |
| // Note: When using BodyStream(), the response body is not copied to memory, | ||
| // so calling Body() afterwards may return an empty slice. |
There was a problem hiding this comment.
The documentation states that calling Body() after BodyStream() may return an empty slice, but this is only true when streaming is enabled. Consider clarifying: 'Note: When streaming is enabled and BodyStream() is used, the response body stream is consumed directly. Subsequent calls to Body() may return incomplete or empty data.'
| // Note: When using BodyStream(), the response body is not copied to memory, | |
| // so calling Body() afterwards may return an empty slice. | |
| // Note: When streaming is enabled (i.e., RawResponse.BodyStream() is non-nil) and BodyStream() is used, | |
| // the response body stream is consumed directly. Subsequent calls to Body() may return incomplete or empty data. | |
| // If streaming is not enabled, Body() will still return the full response body. |
| originalRaw := resp.RawResponse | ||
| resp.RawResponse = respv | ||
| respv = nil | ||
| if originalRaw != nil { | ||
| fasthttp.ReleaseResponse(originalRaw) | ||
| } |
There was a problem hiding this comment.
The logic of replacing resp.RawResponse and releasing originalRaw is complex. Consider adding a comment explaining why this is necessary (to avoid copying the response body when streaming is enabled, the fasthttp.Response object is transferred directly instead of copied).
| func (l *lbClientTransport) StreamResponseBody() bool { | ||
| if len(l.client.Clients) == 0 { | ||
| return false | ||
| } | ||
| // Return the StreamResponseBody setting from the first HostClient | ||
| var streamEnabled bool | ||
| for _, c := range l.client.Clients { | ||
| if walkBalancingClientWithBreak(c, func(hc *fasthttp.HostClient) bool { | ||
| streamEnabled = hc.StreamResponseBody | ||
| return true | ||
| }) { | ||
| break | ||
| } | ||
| } | ||
| return streamEnabled | ||
| } |
There was a problem hiding this comment.
The implementation returns the StreamResponseBody setting from the first HostClient found, but SetStreamResponseBody sets it on all HostClients. This asymmetry could be confusing. Consider adding a comment explaining that all clients should have the same setting, or add validation to ensure consistency.
There was a problem hiding this comment.
Code Review
This pull request introduces excellent support for streaming HTTP response bodies, which is a valuable addition for handling large responses. The implementation is robust, covering both client-level and request-level configurations, and the test coverage is impressively comprehensive. I've identified a few areas for improvement, including some redundant test cases that can be removed for conciseness, a simplification in the Save method to reduce code duplication, and a minor bug in one of the new tests. Overall, this is a high-quality contribution.
| rawStream := resp.RawResponse.BodyStream() | ||
| if rawStream != nil { | ||
| require.True(t, resp.IsStreaming()) | ||
| bodyStream := resp.BodyStream() | ||
| require.NotNil(t, bodyStream) | ||
| require.Same(t, rawStream, bodyStream, "BodyStream() should return the exact same stream object when RawResponse.BodyStream() is not nil") | ||
| } else { | ||
| require.False(t, resp.IsStreaming()) | ||
| bodyStream := resp.BodyStream() | ||
| require.NotNil(t, bodyStream) | ||
| _, ok := bodyStream.(*bytes.Reader) | ||
| require.True(t, ok, "When RawResponse.BodyStream() is nil, BodyStream() should return a *bytes.Reader") | ||
| } | ||
| bodyStream := resp.BodyStream() |
There was a problem hiding this comment.
There's a logical issue here. bodyStream is re-declared outside the if/else block, which results in resp.BodyStream() being called a second time. A response body stream should only be consumed once. This could lead to unexpected behavior or flaky tests. You should acquire the stream once before the conditional checks.
rawStream := resp.RawResponse.BodyStream()
bodyStream := resp.BodyStream()
if rawStream != nil {
require.True(t, resp.IsStreaming())
require.NotNil(t, bodyStream)
require.Same(t, rawStream, bodyStream, "BodyStream() should return the exact same stream object when RawResponse.BodyStream() is not nil")
} else {
require.False(t, resp.IsStreaming())
require.NotNil(t, bodyStream)
_, ok := bodyStream.(*bytes.Reader)
require.True(t, ok, "When RawResponse.BodyStream() is nil, BodyStream() should return a *bytes.Reader")
}| t.Run("with standard client", func(t *testing.T) { | ||
| t.Parallel() | ||
| client := New() | ||
| client.SetStreamResponseBody(true) | ||
| require.True(t, client.StreamResponseBody()) | ||
| }) |
There was a problem hiding this comment.
This test case is redundant as it is functionally identical to the enable streaming test case. Removing it will help keep the test suite concise and easier to maintain.
| t.Run("getter with standard client without setter", func(t *testing.T) { | ||
| t.Parallel() | ||
| client := New() | ||
| // Test getter directly without calling setter | ||
| require.False(t, client.StreamResponseBody()) | ||
| }) |
There was a problem hiding this comment.
This test case is redundant as it is functionally identical to the default value test case. Removing it will help keep the test suite concise and easier to maintain.
| if r.IsStreaming() { | ||
| _, err = io.Copy(outFile, r.BodyStream()) | ||
| } else { | ||
| _, err = io.Copy(outFile, bytes.NewReader(r.Body())) | ||
| } | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to write response body to file: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
|
|
||
| case io.Writer: | ||
| if _, err := io.Copy(p, bytes.NewReader(r.Body())); err != nil { | ||
| return fmt.Errorf("failed to write response body to io.Writer: %w", err) | ||
| var err error | ||
| if r.IsStreaming() { | ||
| _, err = io.Copy(p, r.BodyStream()) | ||
| } else { | ||
| _, err = io.Copy(p, bytes.NewReader(r.Body())) | ||
| } | ||
| defer func() { | ||
| if pc, ok := p.(io.WriteCloser); ok { | ||
| _ = pc.Close() //nolint:errcheck // not needed | ||
| } | ||
| }() | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("failed to write response body to writer: %w", err) | ||
| } |
There was a problem hiding this comment.
The logic for handling streaming vs. non-streaming bodies is duplicated for case string and case io.Writer. This can be simplified since BodyStream() already provides an io.Reader for both cases. You can directly use io.Copy with r.BodyStream() in both branches, which makes the code more concise and maintainable.
| if r.IsStreaming() { | |
| _, err = io.Copy(outFile, r.BodyStream()) | |
| } else { | |
| _, err = io.Copy(outFile, bytes.NewReader(r.Body())) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("failed to write response body to file: %w", err) | |
| } | |
| return nil | |
| case io.Writer: | |
| if _, err := io.Copy(p, bytes.NewReader(r.Body())); err != nil { | |
| return fmt.Errorf("failed to write response body to io.Writer: %w", err) | |
| var err error | |
| if r.IsStreaming() { | |
| _, err = io.Copy(p, r.BodyStream()) | |
| } else { | |
| _, err = io.Copy(p, bytes.NewReader(r.Body())) | |
| } | |
| defer func() { | |
| if pc, ok := p.(io.WriteCloser); ok { | |
| _ = pc.Close() //nolint:errcheck // not needed | |
| } | |
| }() | |
| if err != nil { | |
| return fmt.Errorf("failed to write response body to writer: %w", err) | |
| } | |
| if _, err = io.Copy(outFile, r.BodyStream()); err != nil { | |
| return fmt.Errorf("failed to write response body to file: %w", err) | |
| } | |
| return nil | |
| case io.Writer: | |
| if _, err := io.Copy(p, r.BodyStream()); err != nil { | |
| return fmt.Errorf("failed to write response body to writer: %w", err) | |
| } |
![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.
ℹ️ 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".
| func (l *lbClientTransport) SetStreamResponseBody(enable bool) { | ||
| forEachHostClient(l.client, func(hc *fasthttp.HostClient) { | ||
| hc.StreamResponseBody = enable |
There was a problem hiding this comment.
SetStreamResponseBody only walks the LBClient’s initial Clients slice via forEachHostClient, so host clients added later through fasthttp.LBClient.AddClient (which updates the internal cs list but not Clients) never inherit the streaming setting. In deployments that grow or replace backends at runtime, newly added hosts will keep StreamResponseBody at its default false value and their responses will still be fully buffered, defeating the streaming feature while older hosts stream correctly.
Useful? React with 👍 / 👎.
|
@Abhirup-99 Seems, tests are still failing |
Abhirup-99
force-pushed
the
add-response
branch
from
3c9c80c to
76beedc
Compare
|
@gaby fixed the failing test |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/response_test.go (1)
458-505: Shared ./test directory across parallel tests can cause flaky failuresBoth
Test_Response_Save(“file path” subtest) andTest_Response_Save_Streaming_FilePathwrite under./test/…and each defersos.RemoveAll("./test"). Because these tests/subtests uset.Parallel(), one cleanup can remove the directory while another test still expects its file to exist, making theos.Statchecks intermittently fail.Consider isolating filesystem state per test, for example:
- Use
t.TempDir()and build a unique path inside it for each test, or- Use distinct subdirectories (e.g.
./test/save/…vs./test/save_stream/…) and only remove that subdirectory in each cleanup.This will eliminate cross‑test interference on the same directory.
Also applies to: 1246-1293
♻️ Duplicate comments (4)
client/response.go (2)
92-106: Clarify BodyStream docs for streaming vs non‑streaming casesImplementation is fine, but the comment implies that
Body()may be empty afterBodyStream()in all cases. That’s only true when streaming is enabled (i.e.RawResponse.BodyStream() != nil); in the fallback path you’re just wrappingBody()in abytes.Reader, soBody()stays intact. Consider tightening the note to explicitly say “when streaming is enabled”.
155-177: Simplify Save by always copying from BodyStream()Logic is correct, but the
IsStreaming()branches duplicate the copy paths;BodyStream()already returns a streaming reader when enabled and abytes.ReaderoverBody()otherwise. You can drop the conditional and always doio.Copy(outFile, r.BodyStream())/io.Copy(p, r.BodyStream())to reduce branching and keep Save aligned with the BodyStream contract.client/transport.go (1)
200-221: Document LBClient streaming flag expectations for nested hosts
lbClientTransport.StreamResponseBody()returns the setting from the first reachableHostClient, whileSetStreamResponseBody()applies the flag to all current host clients viaforEachHostClient. That’s reasonable, but it assumes:
- all hosts share the same setting, and
- callers re‑invoke
SetStreamResponseBody()after adding new hosts to an existingLBClient.Consider a brief comment on this assumption or in the client docs so users know they must re‑apply streaming config when mutating the LB’s client set at runtime.
client/response_test.go (1)
720-751: Avoid retaining Body() buffer after Response.Close to prevent flaky testsIn “compare with regular body” you capture:
normalBody := resp1.Body() defer resp1.Close()
Body()returns the underlying fasthttp buffer; afterresp1.Close()the response is returned to a pool and that memory can be reused by other tests, sonormalBodymay later change underneath you. Copy the slice before closing:- normalBody := resp1.Body() + normalBody := append([]byte(nil), resp1.Body()...)This keeps the test stable even when other tests acquire/reuse responses in parallel.
🧹 Nitpick comments (3)
client/client.go (1)
529-541: Client streaming accessors correctly delegate to the transportThese helpers are thin wrappers over the transport’s
StreamResponseBodygetters/setters and keep the public API decoupled from concrete fasthttp clients. One consideration: if callers ever toggle streaming at runtime while requests are in flight, this unsynchronized flag write could race with transport use, similar to other config setters. If you expect dynamic toggling, you may want to reusec.muto guard these like TLS/proxy updates; if it’s “configure once at startup”, the current approach is fine.client/response_test.go (2)
1171-1243: Optionally assert IsStreaming() in the “large response” case
Test_Response_IsStreaming’s “streaming with large response” subtest reads a 64 KB stream but never checksresp.IsStreaming(). Since this test is specifically about the IsStreaming behavior, consider adding:require.True(t, resp.IsStreaming())before consuming the stream to tighten the contract and exercise that branch explicitly.
1295-1376: Save file‑error tests look good; be aware of /dev/full portabilityThe streaming
/dev/fulltest correctly asserts thatSavesurfaces file write/create errors from the OS. Just a minor note:/dev/fullexists and behaves as expected on Linux but may not be present (or may behave differently) on non‑Unix platforms, so if the test suite ever targets Windows, this case might need a platform guard.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
client/client.go(1 hunks)client/client_test.go(1 hunks)client/core.go(2 hunks)client/core_test.go(1 hunks)client/response.go(3 hunks)client/response_test.go(2 hunks)client/transport.go(4 hunks)client/transport_test.go(1 hunks)ctx_test.go(2 hunks)docs/client/rest.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- client/core_test.go
- client/client_test.go
- client/core.go
- client/transport_test.go
- docs/client/rest.md
- ctx_test.go
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
client/response.goclient/response_test.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Applied to files:
client/response.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
client/response.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
client/response_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-10-08T19:06:06.583Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
client/response_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
client/response_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
client/response_test.go
🧬 Code graph analysis (1)
client/response_test.go (1)
client/response.go (3)
Response(19-25)AcquireResponse(222-228)ReleaseResponse(232-235)
🔇 Additional comments (4)
client/transport.go (1)
20-32: Streaming transport accessors correctly forward to fasthttp clientsThe additions to
httpClientTransportand thestandardClientTransport/hostClientTransportimplementations cleanly proxyStreamResponseBodyto the underlying fasthttp types without extra state. This keeps the client‑level API thin and consistent with other transport options.Also applies to: 80-87, 134-141
client/client.go (1)
543-547: SetCookieJar chaining is consistent with other settersReturning
*ClientfromSetCookieJarmatches the fluent style of the rest of the client setters and doesn’t change existing semantics.client/response_test.go (2)
546-619: Streaming Save tests and helpers cover non‑closing and writer error paths wellThe new subtests around
Savewith streaming responses (testClosableBufferandtestErrorWriter) accurately verify that:
Savewrites streamed data without closing user‑provided writers, and- write failures during
io.Copypropagate as wrapped errors.The helper types are minimal and do not leak into production code. This is solid coverage of tricky behaviors for the new Save implementation.
1085-1169: BodyStream fallback tests correctly exercise non‑streaming pathsThe
BodyStream_Fallbackcases (regular body, empty body, and large non‑streaming response) nicely validate that:
IsStreaming()is false when client streaming is disabled, andBodyStream()still provides a working reader over the bufferedBody().No issues spotted here.
|
@gaby bump |
Abhirup-99
force-pushed
the
add-response
branch
from
76beedc to
b7c1cbe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
client/response.go (1)
155-177: Simplify Save() by always using BodyStream()The
if r.IsStreaming()branches are redundant sinceBodyStream()already handles the fallback tobytes.NewReader(r.Body())when streaming is not enabled. This duplication was flagged in past reviews but persists.case string: // ... file creation code ... - if r.IsStreaming() { - _, err = io.Copy(outFile, r.BodyStream()) - } else { - _, err = io.Copy(outFile, bytes.NewReader(r.Body())) - } + _, err = io.Copy(outFile, r.BodyStream()) if err != nil { return fmt.Errorf("failed to write response body to file: %w", err) } return nil case io.Writer: - var err error - if r.IsStreaming() { - _, err = io.Copy(p, r.BodyStream()) - } else { - _, err = io.Copy(p, bytes.NewReader(r.Body())) - } - - if err != nil { + if _, err := io.Copy(p, r.BodyStream()); err != nil { return fmt.Errorf("failed to write response body to writer: %w", err) }client/response_test.go (2)
740-751: Consider copying the body slice to avoid potential flakinessWhile the
defer resp1.Close()runs at function exit (after the comparison), pooling behavior could theoretically cause issues if response pooling is changed. Copying the body is a defensive practice.resp1, err := client1.Get("http://example.com/stream") require.NoError(t, err) defer resp1.Close() - normalBody := resp1.Body() + normalBody := append([]byte(nil), resp1.Body()...)
806-858: Test name doesn't match implementationThe test is named "server sent events with incremental reads" but uses
io.ReadAllwhich reads everything at once. Consider either renaming to "server sent events" or implementing true incremental reads as suggested in past reviews.client/transport.go (1)
200-215: Add clarifying comment about getter/setter asymmetryThe getter returns the setting from the first
HostClientfound, while the setter propagates to all clients. This asymmetry is intentional (all clients should have the same setting), but a brief comment would help maintainers understand the contract.func (l *lbClientTransport) StreamResponseBody() bool { if len(l.client.Clients) == 0 { return false } - // Return the StreamResponseBody setting from the first HostClient + // Return the StreamResponseBody setting from the first HostClient. + // All host clients should have the same value (set via SetStreamResponseBody), + // so returning the first is representative of the entire pool. var streamEnabled bool
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
client/client.go(1 hunks)client/client_test.go(1 hunks)client/core.go(2 hunks)client/core_test.go(1 hunks)client/response.go(3 hunks)client/response_test.go(2 hunks)client/transport.go(4 hunks)client/transport_test.go(1 hunks)ctx_test.go(2 hunks)docs/client/rest.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- client/transport_test.go
- client/core_test.go
- client/client.go
- client/client_test.go
- docs/client/rest.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
client/response_test.goctx_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
client/response_test.goclient/transport.goclient/response.goctx_test.goclient/core.go
🧠 Learnings (14)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
client/response_test.goclient/response.goctx_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
client/response_test.goctx_test.go
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
client/response_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
client/response_test.go
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
client/response_test.go
📚 Learning: 2025-11-26T13:34:08.100Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:34:08.100Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Applied to files:
client/response.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Applied to files:
client/response.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
client/response.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.
Applied to files:
ctx_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
ctx_test.go
📚 Learning: 2025-11-26T13:34:08.100Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:34:08.100Z
Learning: Applies to **/*_test.go : When adding Go tests, always invoke `t.Parallel()` at the start of each test and subtest to maximize concurrency
Applied to files:
ctx_test.go
📚 Learning: 2024-06-30T00:38:06.580Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
ctx_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
ctx_test.go
🧬 Code graph analysis (3)
client/response_test.go (2)
client/client.go (2)
Get(831-833)New(752-757)client/response.go (3)
Response(19-25)AcquireResponse(222-228)ReleaseResponse(232-235)
ctx_test.go (1)
app.go (1)
TestConfig(1115-1126)
client/core.go (1)
client/response.go (1)
ReleaseResponse(232-235)
🔇 Additional comments (9)
ctx_test.go (1)
3951-3954: Explicit TestConfig usage for app.Test looks goodUsing
TestConfig{Timeout: 5 * time.Second}for these file‑upload tests is consistent with the new API, gives more headroom than the default 1s timeout, and still fails on real hangs via the defaultFailOnTimeout: true. Tests already callt.Parallel()at the top, so they comply with the test guidelines.Also applies to: 3993-3996
client/response.go (1)
92-106: LGTM!The
BodyStream()andIsStreaming()implementations are clean. The fallback tobytes.NewReaderwhen streaming is not enabled ensures the API remains consistent regardless of streaming state.client/response_test.go (3)
622-654: LGTM!The test helper types
testClosableBufferandtestErrorWriterare well-implemented for their purposes - tracking close behavior and simulating write errors respectively.
1042-1082: LGTM!The test correctly validates stream identity by comparing object references without consuming the stream. The subsequent
io.ReadAllproperly consumes the stream after identity verification.
1340-1391: LGTM!Good cross-platform test design - handles Linux
/dev/full, and creates read-only files on other systems. Usest.TempDir()for automatic cleanup.client/transport.go (1)
30-31: LGTM!The interface additions follow the established getter/setter pattern used for other transport configurations like
TLSConfig.client/core.go (3)
122-130: LGTM! Consider adding a brief comment explaining the swap patternThe response swap pattern correctly avoids copying by transferring ownership of
respvtoresp.RawResponseand releasing the pre-acquired placeholder. Settingrespv = nilprevents the deferred release from double-freeing.A brief inline comment would help future maintainers understand this ownership transfer:
resp := AcquireResponse() resp.setClient(c.client) resp.setRequest(c.req) + // Transfer respv ownership to resp, release the placeholder from AcquireResponse originalRaw := resp.RawResponse resp.RawResponse = respv - respv = nil + respv = nil // Prevent double-release in defer if originalRaw != nil { fasthttp.ReleaseResponse(originalRaw) }
95-98: LGTM!Correctly forwards the request's
BodyStreamafterCopyTo, sinceCopyTodoesn't transfer streams. UsingContentLength()from the header ensures proper stream handling.
88-93: LGTM!The nil-guard in the deferred release is necessary since
respvis set tonilafter successful ownership transfer to prevent double-release.
Abhirup-99
force-pushed
the
add-response
branch
from
b7c1cbe to
0dfe1c3
Compare
|
added tests based on github copilot, don't have access to a windows machine. Also, I didn't make any changes in the middlewares which seem to be failing in the benchmark |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
client/client_test.go (2)
2373-2378: Redundant test case: functionally identical to "enable streaming".This test case performs the same operations as the "enable streaming" test (lines 2356-2362): creates a new client, enables streaming, and verifies
StreamResponseBody()returnstrue. Consider removing to reduce test maintenance overhead.
2401-2406: Redundant test case: functionally identical to "default value".This test case performs the same check as the "default value" test (lines 2350-2354): creates a new client and verifies
StreamResponseBody()returnsfalse. Consider removing to keep the test suite concise.client/transport.go (1)
200-221: Acknowledged asymmetry: getter reads first client, setter propagates to all.The getter returns the setting from the first discovered HostClient, while the setter propagates to all. This is acceptable since:
- All clients should have the same setting after SetStreamResponseBody
- The getter assumption is that settings are uniform
Consider adding a brief comment noting this design assumption:
func (l *lbClientTransport) StreamResponseBody() bool { if len(l.client.Clients) == 0 { return false } - // Return the StreamResponseBody setting from the first HostClient + // Return the StreamResponseBody setting from the first HostClient. + // All host clients are expected to have the same setting after SetStreamResponseBody. var streamEnabled boolNote: As previously identified, dynamically added clients via
fasthttp.LBClient.AddClientwill not inherit the streaming setting. This is a known limitation of the current design.client/response_test.go (1)
1042-1082: Acquire BodyStream once before conditional checks to avoid double consumption.
resp.BodyStream()is called once inside the if/else branches (lines 1065, 1070) and again after (line 1075). A response body stream should be consumed only once. Acquire it once before the conditional and reuse it.Apply this diff:
rawStream := resp.RawResponse.BodyStream() + bodyStream := resp.BodyStream() if rawStream != nil { require.True(t, resp.IsStreaming()) - bodyStream := resp.BodyStream() require.NotNil(t, bodyStream) require.Same(t, rawStream, bodyStream, "BodyStream() should return the exact same stream object when RawResponse.BodyStream() is not nil") } else { require.False(t, resp.IsStreaming()) - bodyStream := resp.BodyStream() require.NotNil(t, bodyStream) _, ok := bodyStream.(*bytes.Reader) require.True(t, ok, "When RawResponse.BodyStream() is nil, BodyStream() should return a *bytes.Reader") } - bodyStream := resp.BodyStream() data, err := io.ReadAll(bodyStream)
🧹 Nitpick comments (5)
docs/client/rest.md (1)
409-423: Documentation is functional but could benefit from additional detail.The documentation for
StreamResponseBodyandSetStreamResponseBodyis minimal. Consider adding:
- The default value (
false)- How to override at request level via
Request.SetStreamResponseBody(bool)- A note that when streaming is enabled,
Response.Body()may return empty data—useResponse.BodyStream()instead- Reference to
Response.IsStreaming()to check current stateThis aligns with the past review suggestions that were marked as addressed, but the current text doesn't reflect those additions.
client/core.go (1)
125-130: Add a comment explaining the response transfer pattern.This logic is critical for streaming support but non-obvious. Consider adding a brief comment explaining why we transfer ownership rather than copying:
+ // Transfer the fasthttp.Response directly to preserve streaming semantics. + // CopyTo would buffer the body, breaking BodyStream support. originalRaw := resp.RawResponse resp.RawResponse = respv respv = nil if originalRaw != nil { fasthttp.ReleaseResponse(originalRaw) }client/response.go (1)
155-177: Consider simplifying by always usingBodyStream().Since
BodyStream()already handles the fallback tobytes.NewReader(r.Body())when not streaming, the explicitIsStreaming()branches are redundant:- if r.IsStreaming() { - _, err = io.Copy(outFile, r.BodyStream()) - } else { - _, err = io.Copy(outFile, bytes.NewReader(r.Body())) - } + _, err = io.Copy(outFile, r.BodyStream())This would also apply to the
io.Writercase. The current explicit branching isn't incorrect—it may be intentional for clarity—but it duplicates logic already inBodyStream().client/response_test.go (2)
720-751: Consider copying normalBody before comparison to avoid potential flakiness.While
defer resp1.Close()doesn't fire until function exit (after the comparison), retaining a reference to the internal buffer can theoretically cause flakiness if the pool aggressively reuses memory. Copying the bytes is a safer pattern.Apply this diff for extra safety:
defer resp1.Close() - normalBody := resp1.Body() + normalBody := append([]byte(nil), resp1.Body()...)
911-972: Clarify expected behavior in connection interruption test.The test documents that reading after close has undefined behavior (may error or panic), which is acceptable. However, line 917's
ImmediateHeaderFlushis set but may have no effect sinceSendStreamWriteralready handles streaming. Consider removing if redundant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
client/client.go(1 hunks)client/client_test.go(1 hunks)client/core.go(2 hunks)client/core_test.go(1 hunks)client/response.go(3 hunks)client/response_test.go(2 hunks)client/transport.go(4 hunks)client/transport_test.go(1 hunks)ctx_test.go(2 hunks)docs/client/rest.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- client/client.go
- client/core_test.go
- ctx_test.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
When adding Go tests, always invoke
t.Parallel()at the start of each test and subtest to maximize concurrency
Files:
client/transport_test.goclient/response_test.goclient/client_test.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Prefer
github.com/gofiber/utils/v2helpers (for example,utils.Trim) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Files:
client/transport_test.goclient/response_test.goclient/client_test.goclient/response.goclient/core.goclient/transport.go
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Review and update the contents of the
docsfolder if necessary when modifying code
Files:
docs/client/rest.md
**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
make markdownto lint all Markdown files when modifying code
Files:
docs/client/rest.md
🧠 Learnings (10)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.
Applied to files:
client/response_test.goclient/client_test.goclient/response.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
Applied to files:
client/response_test.goclient/client_test.go
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.
Applied to files:
client/response_test.goclient/client_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
client/response_test.go
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
client/response_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
client/client_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
client/client_test.go
📚 Learning: 2025-11-26T13:34:08.100Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-26T13:34:08.100Z
Learning: Applies to **/*.go : Prefer `github.com/gofiber/utils/v2` helpers (for example, `utils.Trim`) when performing common operations such as string manipulation, whenever it is practical and appropriate for the surrounding code
Applied to files:
client/response.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Applied to files:
client/response.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.
Applied to files:
client/response.go
🔇 Additional comments (18)
client/client_test.go (1)
2347-2399: LGTM! Test structure follows conventions.The test properly invokes
t.Parallel()at the start of the main test and each subtest, as required by coding guidelines. The tests cover key scenarios: default value, enable/disable toggling, and propagation to host and LB clients.client/core.go (2)
89-93: LGTM! Proper nil guard for response release.The nil check before
ReleaseResponse(respv)is necessary becauserespvis set tonilon line 127 when the response is transferred toresp.RawResponse. This prevents double-release and ensures proper resource cleanup.
96-98: LGTM! BodyStream propagation preserves streaming across retries.When the original request has a
BodyStream, forwarding it to the fasthttp request viaSetBodyStreamensures streaming body content is preserved through retries and redirects.client/response.go (1)
92-106: LGTM! Clean streaming API.The
BodyStream()method correctly falls back to abytes.Readerwhen streaming isn't enabled, providing a consistentio.Readerinterface.IsStreaming()offers a simple check for callers who need to know the response mode.client/transport_test.go (3)
350-378: LGTM! Comprehensive streaming tests for standard transport.Tests cover default value, enable, and disable paths with proper verification that the underlying
fasthttp.Client.StreamResponseBodyfield is updated.
411-508: LGTM! Thorough LB transport streaming tests.Excellent coverage of edge cases including empty clients, single host, multiple hosts, nested LB clients, and mixed clients with stubs. The nested LB test (lines 467-488) correctly verifies that streaming state propagates through wrapper layers to underlying host clients.
510-548: LGTM! Interface compliance test ensures consistent behavior.This test validates that all transport implementations (
standardClientTransport,hostClientTransport,lbClientTransport) properly implement thehttpClientTransportinterface with consistent streaming toggle behavior.client/response_test.go (8)
545-619: LGTM! Streaming test coverage for Save() with io.Writer.These tests properly validate streaming behavior:
- Uses
c.SendStreamWriterto ensure server-side streaming- Tests that Save() does not close the provided writer
- Tests error handling during streaming copy
Good use of helper types (
testClosableBuffer,testErrorWriter) for controlled test scenarios.
622-654: Well-designed test helper types.Both helpers serve clear purposes:
testClosableBuffer: Tracks whether Close() was called, useful for verifying resource managementtestErrorWriter: Simulates write failures at specific byte counts for error path testing
656-679: LGTM! Basic streaming test is well-structured.Correctly uses
c.SendStream()for server-side streaming and validates the complete flow from client setup through stream consumption.
806-858: LGTM! SSE test now uses proper streaming via SendStreamWriter.The test correctly uses
c.SendStreamWriterwith flushing between messages to exercise true server-side streaming. This addresses previous feedback about ensuring the server actually streams data.
1085-1169: LGTM! Comprehensive fallback tests for non-streaming scenarios.Good coverage of edge cases:
- Non-streaming response correctly returns
*bytes.Reader- Empty response body handled properly
- Large non-streaming responses work correctly with BodyStream()
1171-1244: LGTM! IsStreaming state tests are thorough.Tests correctly validate:
- Streaming enabled with large responses
- Streaming disabled returns false
- BodyStream() works consistently regardless of streaming state
1317-1338: LGTM! Pool acquire/release test validates basic lifecycle.Good test for ensuring the response pool works correctly. The comment on line 1333-1334 appropriately notes the limitation of testing pooled objects after release.
1340-1391: Good cross-platform write error testing.The test appropriately handles platform differences:
- Uses
/dev/fullon Linux for write errors- Falls back to read-only file approach on other systems
The
//nolintcomments are appropriate given the intentional test file operations.client/transport.go (3)
30-31: LGTM! Clean interface extension for streaming controls.The new methods follow the established pattern of the interface and provide symmetric getter/setter access.
80-86: LGTM! Straightforward delegation to fasthttp.Client.Direct field access is appropriate here since
fasthttp.Client.StreamResponseBodyis a public field.
134-140: LGTM! Consistent implementation for HostClient transport.Mirrors the standardClientTransport implementation appropriately.
|
Ok thx, will check your pr tomorrow again |
6 participants
This pull request introduces support for streaming HTTP response bodies in the client, allowing responses to be read as streams rather than being fully loaded into memory. This is particularly useful for handling large responses or server-sent events. The changes include new configuration options at both the client and request levels, implementation of the streaming logic, and comprehensive tests to ensure correct behavior.
Streaming response body support:
streamResponseBodyfield to theClientstruct, along withSetStreamResponseBodyandStreamResponseBodymethods to enable or disable response body streaming at the client level (client/client.go). [1] [2]streamResponseBodyfield to theRequeststruct, with correspondingSetStreamResponseBodyandStreamResponseBodymethods to allow per-request configuration that overrides the client setting (client/request.go). [1] [2]client/core.go).BodyStreammethod on theResponsestruct to provide anio.Readerfor streaming the response body, falling back to an in-memory reader if streaming is not enabled (client/response.go).Testing and validation:
client/client_test.go,client/response_test.go). [1] [2]streamResponseBodyoverride (client/request.go).# DescriptionPlease provide a clear and concise description of the changes you've made and the problem they address. Include the purpose of the change, any relevant issues it solves, and the benefits it brings to the project. If this change introduces new features or adjustments, highlight them here.
Related #3425