🐛 fix: avoid writing into released Response in core::execFunc() #3830
Conversation
|
Warning Rate limit exceeded@arturmelanchyk has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
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. WalkthroughReplaces atomic-based synchronization in request execution with channel-based coordination and channel pooling; refactors execFunc to use per-call goroutines and pooled response/error channels. Adds tests and a benchmark covering context cancellation, redirect-following with retries, and a blocking transport mock. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Client
participant Exec as execFunc
participant PoolE as errChanPool
participant PoolR as respChanPool
participant Worker as Request Goroutine
participant Trans as Transport
Caller->>Exec: Call with context, retries, redirects
Exec->>PoolE: acquireErrChan()
Exec->>PoolR: acquireResponseChan()
Exec->>Worker: spawn with errCh/respCh
par transport
Worker->>Trans: perform request
alt success
Trans-->>Worker: response
Worker->>respCh: send response
else error
Trans-->>Worker: error
Worker->>errCh: send error
end
and context-monitor
Caller->>Exec: context cancelled?
alt cancelled before result
Exec->>errCh: drain / ignore pending sends
end
end
Worker->>PoolR: releaseResponseChan()
Worker->>PoolE: releaseErrChan()
Exec-->>Caller: return response or ErrTimeoutOrCancel / other error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to review closely:
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3830 +/- ##
=======================================
Coverage 92.25% 92.26%
=======================================
Files 115 115
Lines 9734 9745 +11
=======================================
+ Hits 8980 8991 +11
Misses 480 480
Partials 274 274
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:
|
arturmelanchyk
force-pushed
the
exec-func
branch
from
4872dbd to
8bea55c
Compare
arturmelanchyk
changed the title
arturmelanchyk
force-pushed
the
exec-func
branch
3 times, most recently
from
2d79c28 to
70aeab8
Compare
Signed-off-by: Artur Melanchyk <[email protected]>
Signed-off-by: Artur Melanchyk <[email protected]>
Signed-off-by: Artur Melanchyk <[email protected]>
arturmelanchyk
force-pushed
the
exec-func
branch
from
70aeab8 to
f7303ac
Compare
arturmelanchyk
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/core.go (1)
233-237: Clarify panic text for response channel assertion.The panic string still says
*Response, even though the type assertion is forchan *Response, which can send someone debugging pool corruption down the wrong path. Please update the message to reflect the actual type being asserted.- panic(errors.New("failed to type-assert to *Response")) + panic(errors.New("failed to type-assert to chan *Response"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
client/client_test.go(1 hunks)client/core.go(3 hunks)client/core_test.go(5 hunks)
🧰 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_test.goclient/core_test.go
🧠 Learnings (10)
📓 Common learnings
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.
📚 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/core.goclient/core_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/core_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/core_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/core_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/core_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/core_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:
client/core_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/core_test.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/core_test.go
🧬 Code graph analysis (3)
client/core.go (2)
client/request.go (2)
AcquireRequest(983-989)ReleaseRequest(993-996)client/response.go (3)
AcquireResponse(197-203)ReleaseResponse(207-210)Response(19-25)
client/client_test.go (3)
client/client.go (1)
New(738-743)client/request.go (3)
AcquireRequest(983-989)Request(46-73)ReleaseRequest(993-996)client/core.go (1)
ErrTimeoutOrCancel(281-281)
client/core_test.go (4)
client/client.go (4)
Get(817-819)New(738-743)Config(641-655)Client(37-67)client/request.go (3)
AcquireRequest(983-989)Header(708-710)Request(46-73)client/core.go (2)
RetryConfig(31-31)ErrTimeoutOrCancel(281-281)client/response.go (1)
Response(19-25)
|
/gemini review @codex review |
|
Thank you for invoking the review, @gaby! I've completed my review of this pull request. This PR effectively addresses a critical race condition in Overall AssessmentThis is a well-executed refactor that improves the correctness and robustness of the client library. The introduction of channel pooling for response and error channels is a good practice for resource management in Go. The new test utilities and benchmark additions are valuable for validating the changes and measuring performance. Review Highlights
Minor Suggestion
Overall, this is a strong pull request that significantly improves the reliability and performance of the client. Great work! |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the execFunc method to fix a concurrency issue related to context cancellation and channel draining. The key changes replace the atomic flag pattern with separate response and error channels, ensuring proper cleanup when a request is canceled.
- Replaces atomic done flag with separate
respChananderrChanfor cleaner concurrency handling - Adds a goroutine to drain channels and release resources when context is canceled
- Changes channel buffers from buffered to unbuffered for both error and response channels
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| client/core.go | Refactored execFunc() to use separate response and error channels with proper draining on context cancellation; added response channel pool infrastructure |
| client/core_test.go | Added tests for redirect with retry config and context cancellation draining; introduced blockingErrTransport mock for testing concurrent scenarios |
| client/client_test.go | Added benchmark test for context cancellation to measure performance impact of the new draining mechanism |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@arturmelanchyk Can you fix the notes for client/core.go. I'm not worried about the tests comment. |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Artur Melanchyk <[email protected]>
arturmelanchyk
force-pushed
the
exec-func
branch
from
564e37e to
61b4051
Compare
2 participants
Description
This PR addresses issue in
core::execFunc()where goroutine can potentially write intoResponseafter it has been released back to the pool.fiber/client/core.go
Lines 75 to 134 in 6ef6de5
On line 111 when
doneis set in theifstatement:fiber/client/core.go
Line 111 in 6ef6de5
and
contextcancellation happens concurrently at this stage, thenatomic.SwapInt32(&done, 1)at line 130:fiber/client/core.go
Lines 129 to 131 in 6ef6de5
will unconditionally overwrite
donevalue and releaseResponse. As a result execution at line 112:fiber/client/core.go
Line 116 in 6ef6de5
can still write to Response even when it is returned back to the pool.
Alternatively, if http request fails, then
errChwill be released back to the pool while goroutine still trying to write into it:fiber/client/core.go
Lines 111 to 113 in 6ef6de5
Additionally, by removing unneeded
doneatomic I achieved ~3% performance boost in single request benchmark and ~8% in parallel.Context cancellation path shows slightly degraded performance because now we also correctly drain error chan that was previously missing
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md