feat(prefork): delegate to fasthttp/prefork to eliminate code duplication#4210
feat(prefork): delegate to fasthttp/prefork to eliminate code duplication#4210ReneWerner87 wants to merge 21 commits intomainfrom
Conversation
…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>
Use github.com/ReneWerner87/fasthttp@2802b1a (prefork_optimization branch) instead of local ../fasthttp path, so CI and other developers can resolve the dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Includes fix for ListenAndServeTLS parameter order preservation. 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>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
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.
…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>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4210 +/- ##
==========================================
+ Coverage 91.16% 91.28% +0.11%
==========================================
Files 123 127 +4
Lines 11855 12321 +466
==========================================
+ Hits 10808 11247 +439
- Misses 659 672 +13
- Partials 388 402 +14
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:
|
Includes lint fixes and review feedback for CommandProducer validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
OnChildRecover no longer returns an error (simplified API). Update fasthttp fork reference to 9758f93. 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>
Includes zombie process fix and lifecycle integration test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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: 4ad12cd | Previous: dd8ec01 | Ratio |
|---|---|---|---|
Benchmark_Ctx_Cookie (github.com/gofiber/fiber/v3) |
189.4 ns/op 0 B/op 0 allocs/op |
122 ns/op 0 B/op 0 allocs/op |
1.55 |
Benchmark_Ctx_Cookie (github.com/gofiber/fiber/v3) - ns/op |
189.4 ns/op |
122 ns/op |
1.55 |
This comment was automatically generated by workflow using github-action-benchmark.
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.
Summary
Replaces previous PR #4037 (accidentally merged via branch tracking misconfiguration).
preforkpackagePreforkRecoverThresholdtoListenConfigfor configurable child recoveryPreforkLoggertoListenConfigfor configurable prefork loggingIsChild()delegates toprefork.IsChild()prefork_logger.gowithPreforkLoggerInterfaceDepends on
valyala/fasthttp#2180
Test plan
🤖 Generated with Claude Code