🔥 feat(prefork): delegate implementation to fasthttp/prefork#4210
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPrefork orchestration is migrated to github.com/valyala/fasthttp/prefork. ListenConfig gains ChangesPrefork Migration to fasthttp
Sequence Diagram(s)sequenceDiagram
participant Test
participant App
participant PreforkLib
participant Child
Test->>App: enableTestPreforkMaster / call Listen with threshold 1
App->>PreforkLib: Configure & Start Prefork
PreforkLib->>Child: Spawn -> ServeFunc
Child->>App: startupProcess -> server.Serve
PreforkLib-->>Test: return prefork.ErrOverRecovery on over-recovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Code Review
This pull request refactors the preforking mechanism by delegating process management to the fasthttp/prefork package, simplifying the internal implementation and adding support for recovery thresholds and custom loggers. Feedback highlights a security and maintenance risk regarding the use of a personal fork for fasthttp, a bug in the test command producer that starts processes prematurely, and a logic issue where the default recovery threshold could be zero on single-core systems.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4210 +/- ##
==========================================
- Coverage 91.21% 91.20% -0.01%
==========================================
Files 129 130 +1
Lines 12767 12753 -14
==========================================
- Hits 11645 11632 -13
+ Misses 708 707 -1
Partials 414 414
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:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 679f53b | Previous: 5a8c2fd | Ratio |
|---|---|---|---|
Benchmark_Ctx_IP (github.com/gofiber/fiber/v3) |
59.65 ns/op 8 B/op 1 allocs/op |
38.89 ns/op 8 B/op 1 allocs/op |
1.53 |
Benchmark_Ctx_IP (github.com/gofiber/fiber/v3) - ns/op |
59.65 ns/op |
38.89 ns/op |
1.53 |
Benchmark_Ctx_IsProxyTrusted/WithProxyCheckSimple (github.com/gofiber/fiber/v3) |
67.64 ns/op 8 B/op 1 allocs/op |
44.48 ns/op 8 B/op 1 allocs/op |
1.52 |
Benchmark_Ctx_IsProxyTrusted/WithProxyCheckSimple (github.com/gofiber/fiber/v3) - ns/op |
67.64 ns/op |
44.48 ns/op |
1.52 |
Benchmark_Redirect_Message (github.com/gofiber/fiber/v3) |
49.34 ns/op 0 B/op 0 allocs/op |
31.62 ns/op 0 B/op 0 allocs/op |
1.56 |
Benchmark_Redirect_Message (github.com/gofiber/fiber/v3) - ns/op |
49.34 ns/op |
31.62 ns/op |
1.56 |
This comment was automatically generated by workflow using github-action-benchmark.
…tion Replace Fiber's own prefork implementation with fasthttp's prefork package. This eliminates duplicated process management logic and adds new features: - IsChild() now delegates to prefork.IsChild() - Master process uses fasthttp's recovery loop (RecoverThreshold) - Callbacks (OnChildSpawn, OnMasterReady, OnChildRecover) integrate Fiber's hooks system (OnFork, OnListen, startup message) - CommandProducer enables clean test injection (replaces testPreforkMaster) - OnMasterDeath replaces Fiber's watchMaster (now in fasthttp) - New PreforkRecoverThreshold field in ListenConfig - prefork_logger.go adapts Fiber's logger to fasthttp's Logger interface - go.mod uses replace directive for local fasthttp development Code reduction: ~200 lines of process management removed from Fiber. All existing tests adapted for recovery behavior (ErrOverRecovery). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ensures deterministic and fast execution regardless of GOMAXPROCS count. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- prefork_logger.go: use grouped import declaration - prefork.go: rename unused 'files' parameter to '_' - prefork.go: wrap external errors from cmd.Start() and ListenAndServe() Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move windowsOS constant to constants.go (global consts file) - Make prefork logger configurable via ListenConfig.PreforkLogger - Define PreforkLoggerInterface for custom logger implementations - Use logger.Printf instead of direct log.Warnf in OnChildRecover - Remove unused log import from prefork.go Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tems runtime.GOMAXPROCS(0)/2 yields 0 on single-core, disabling recovery. Use max(1, ...) to guarantee at least one recovery attempt. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
OnChildRecover now receives both the crashed and replacement PID. OnFork hooks for recovered children are handled by OnChildSpawn which fasthttp now calls for recoveries too. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mirrors fasthttp PR #2180 which made OnChildRecover symmetric with the other lifecycle hooks. Fiber's recovery callback only logs and so simply returns nil; future callers can short-circuit recovery by returning an error from this hook. Also renames the local pid parameters to oldPID/newPID per Go initialism convention.
ReneWerner87
force-pushed
the
prefork_optimization
branch
from
8050849 to
04a5894
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors Fiber’s prefork support to delegate process management to github.com/valyala/fasthttp/prefork, aiming to reduce duplicated prefork logic while adding configuration knobs for child recovery and prefork logging.
Changes:
- Replaced Fiber’s custom prefork process management with
fasthttp/preforkintegration (callbacks + ServeFunc wrapper). - Added
PreforkRecoverThresholdandPreforkLoggertoListenConfig, plus a new prefork logger adapter interface. - Updated prefork-related tests and documentation to reflect the new behavior and configuration.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| prefork.go | Delegates prefork process management to fasthttp/prefork and wires Fiber hooks/logging into callbacks. |
| prefork_test.go | Adjusts prefork tests for the new fasthttp/prefork error behavior and adds coverage for IsChild and logger adapter. |
| prefork_logger.go | Introduces a prefork logger interface and default adapter to Fiber’s logger. |
| listen.go | Extends ListenConfig with prefork recovery/logging configuration fields. |
| listen_test.go | Updates prefork listen tests to expect fasthttp/prefork recovery error semantics. |
| hooks_test.go | Updates prefork hook tests to the new prefork execution model. |
| docs/api/fiber.md | Documents new prefork-related ListenConfig fields. |
| constants.go | Introduces a windowsOS identifier constant used by prefork test helpers. |
|
/gemini review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prefork.go (1)
68-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefork currently skips
BeforeServeFunc.The normal
Listenpath still invokescfg.BeforeServeFunc(app)right beforeServe, but the new prefork child path goes straight toapp.server.Serve(ln). Any setup wired throughBeforeServeFuncnow silently stops running whenEnablePreforkis enabled.💡 Suggested fix
p.ServeFunc = func(ln net.Listener) error { // wrap a tls config around the listener if provided if tlsConfig != nil { ln = tls.NewListener(ln, tlsConfig) } // prepare the server for the start app.startupProcess() if cfg.ListenerAddrFunc != nil { cfg.ListenerAddrFunc(ln.Addr()) } + + if cfg.BeforeServeFunc != nil { + if err := cfg.BeforeServeFunc(app); err != nil { + return err + } + } // listen for incoming connections return app.server.Serve(ln) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prefork.go` around lines 68 - 84, The prefork child ServeFunc omits invoking cfg.BeforeServeFunc, so ensure you call it (if non-nil) before starting the server in p.ServeFunc; specifically, inside the ServeFunc (where app.startupProcess() is called) add a nil-check and call cfg.BeforeServeFunc(app) prior to returning app.server.Serve(ln) so any registered setup runs in the prefork child just like the normal Listen path.
🧹 Nitpick comments (1)
prefork.go (1)
15-21: ⚡ Quick winMake the prefork test seam state self-cleaning.
These package-level knobs are now part of the runtime path, and the new tests flip them without guaranteed cleanup. That makes prefork tests order-dependent and race-prone if more of them ever go parallel. A small helper that sets the seam and registers
t.Cleanupwould keep this deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prefork.go` around lines 15 - 21, The package-level test seams (testPreforkMaster, testOnPrefork, dummyPid, dummyChildCmd) are global and can leak state across tests; add a small helper function (e.g., SetPreforkTestSeam or WithPreforkTestSeam) that accepts the desired values (or a struct) and a *testing.T, sets those globals (including storing previous values), and registers t.Cleanup to restore the previous values (and clear dummyChildCmd) when the test finishes; update tests to call this helper instead of mutating the globals directly so state becomes self-cleaning and safe for parallel/ordered runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api/fiber.md`:
- Around line 122-123: The docs table for PreforkRecoverThreshold is incorrect
on single-core systems; update the description and default value for
PreforkRecoverThreshold to reflect the clamped implementation (it uses max(1,
runtime.GOMAXPROCS(0)/2)). Locate the row referencing PreforkRecoverThreshold in
docs/api/fiber.md and change the default column and description to state that
the default is max(1, runtime.GOMAXPROCS(0)/2) (i.e., at least 1 on
single-core), keeping the existing note that it only applies when prefork is
enabled.
---
Outside diff comments:
In `@prefork.go`:
- Around line 68-84: The prefork child ServeFunc omits invoking
cfg.BeforeServeFunc, so ensure you call it (if non-nil) before starting the
server in p.ServeFunc; specifically, inside the ServeFunc (where
app.startupProcess() is called) add a nil-check and call
cfg.BeforeServeFunc(app) prior to returning app.server.Serve(ln) so any
registered setup runs in the prefork child just like the normal Listen path.
---
Nitpick comments:
In `@prefork.go`:
- Around line 15-21: The package-level test seams (testPreforkMaster,
testOnPrefork, dummyPid, dummyChildCmd) are global and can leak state across
tests; add a small helper function (e.g., SetPreforkTestSeam or
WithPreforkTestSeam) that accepts the desired values (or a struct) and a
*testing.T, sets those globals (including storing previous values), and
registers t.Cleanup to restore the previous values (and clear dummyChildCmd)
when the test finishes; update tests to call this helper instead of mutating the
globals directly so state becomes self-cleaning and safe for parallel/ordered
runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6ba5a78-16ba-498d-aab8-8101b29b40de
📒 Files selected for processing (8)
constants.godocs/api/fiber.mdhooks_test.golisten.golisten_test.goprefork.goprefork_logger.goprefork_test.go
There was a problem hiding this comment.
Code Review
This pull request refactors the prefork implementation by delegating process management to the fasthttp/prefork package, which simplifies the codebase and improves reliability. It introduces new configuration options, PreforkRecoverThreshold and PreforkLogger, to the ListenConfig struct, providing better control over child process recovery and logging. The test suite has been updated to validate these changes, specifically ensuring that the master process correctly handles recovery limits. Feedback from the review suggests aligning the documentation with the implementation's minimum recovery threshold and refactoring the logger initialization to improve readability and remove linter suppressions.
|
Fixes #1208 |
gaby
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
middleware/logger/logger_test.go (2)
663-663:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
t.Parallel()to enable concurrent test execution.As per coding guidelines, Go tests should invoke
t.Parallel()at the start to maximize concurrency. This test is safe to parallelize—it uses a dedicated app instance, thread-safe buffer pooling, and synchronousapp.Testcalls.�parallel Proposed fix
func Test_Logger_WithLatency(t *testing.T) { + t.Parallel() buff := bytebufferpool.Get()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware/logger/logger_test.go` at line 663, The test function Test_Logger_WithLatency is missing t.Parallel(), so modify the Test_Logger_WithLatency function to call t.Parallel() immediately at the start of the test; locate the Test_Logger_WithLatency declaration and insert a single t.Parallel() invocation as the first statement inside that function to enable concurrent test execution.
711-711:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
t.Parallel()to enable concurrent test execution.As per coding guidelines, Go tests should invoke
t.Parallel()at the start. This test is safe to parallelize for the same reasons asTest_Logger_WithLatency: dedicated app instance, thread-safe primitives, and synchronous execution.�parallel Proposed fix
func Test_Logger_WithLatency_DefaultFormat(t *testing.T) { + t.Parallel() buff := bytebufferpool.Get()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middleware/logger/logger_test.go` at line 711, Add t.Parallel() as the first statement inside the Test_Logger_WithLatency_DefaultFormat function to enable concurrent test execution; locate the Test_Logger_WithLatency_DefaultFormat test function and insert a call to t.Parallel() at the start (similar to Test_Logger_WithLatency) to make the test run safely in parallel with other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@middleware/logger/logger_test.go`:
- Line 663: The test function Test_Logger_WithLatency is missing t.Parallel(),
so modify the Test_Logger_WithLatency function to call t.Parallel() immediately
at the start of the test; locate the Test_Logger_WithLatency declaration and
insert a single t.Parallel() invocation as the first statement inside that
function to enable concurrent test execution.
- Line 711: Add t.Parallel() as the first statement inside the
Test_Logger_WithLatency_DefaultFormat function to enable concurrent test
execution; locate the Test_Logger_WithLatency_DefaultFormat test function and
insert a call to t.Parallel() at the start (similar to Test_Logger_WithLatency)
to make the test run safely in parallel with other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5b49303e-e535-401d-bacd-8ef1ee748695
📒 Files selected for processing (1)
middleware/logger/logger_test.go
Summary
Replaces previous PR #4037 (accidentally merged via branch tracking misconfiguration).
preforkpackagePreforkRecoverThresholdtoListenConfigfor configurable child recoveryPreforkLoggertoListenConfigfor configurable prefork loggingIsChild()delegates toprefork.IsChild()prefork_logger.gowithPreforkLoggerDepends on
github.com/valyala/fasthttp v1.71.0, which includes the required prefork callbacks from feat(prefork): Enhance prefork management with WatchMaster, CommandProducer, and Windows support valyala/fasthttp#2180Test plan
go test -count=1 -run 'Test_App_Prefork|Test_IsChild|Test_Prefork_Logger|Test_Hook_OnListenPrefork|Test_Hook_OnHook|Test_Listen_Prefork|Test_Listen_TLSMinVersion|Test_Listen_TLS_Prefork|Test_Listen_MutualTLS_Prefork' .go test -race -shuffle=on -count=1 ./...golangci-lint v2.6.2 run