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 7 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)
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.
| gopkg.in/yaml.v3 v3.0.1 // indirect | ||
| ) | ||
|
|
||
| replace github.com/valyala/fasthttp => github.com/ReneWerner87/fasthttp v0.0.0-20260411181936-c1055ce62c37 |
There was a problem hiding this comment.
The replace directive points to a personal fork of fasthttp (ReneWerner87/fasthttp). Depending on a personal fork for core process management logic in a major framework like Fiber is a significant maintenance and security risk. This change should ideally wait until the required features are merged into the official valyala/fasthttp repository to ensure long-term stability and trust.
| if err := cmd.Start(); err != nil { | ||
| return cmd, fmt.Errorf("prefork: failed to start test command: %w", err) | ||
| } | ||
| return fmt.Errorf("prefork: %w", err) | ||
| return cmd, nil |
There was a problem hiding this comment.
Calling cmd.Start() inside the CommandProducer is likely a bug. An *exec.Cmd can only be started once. Since fasthttp/prefork is responsible for managing the child processes, it will almost certainly call Start() on the command returned by this producer. If it does, the second attempt to start the process will fail with an "exec: already started" error. The producer should only configure and return the command, leaving the execution to the manager.
return cmd, nil| // Determine RecoverThreshold | ||
| recoverThreshold := cfg.PreforkRecoverThreshold | ||
| if recoverThreshold == 0 { | ||
| recoverThreshold = runtime.GOMAXPROCS(0) / 2 |
There was a problem hiding this comment.
The default recoverThreshold calculation (runtime.GOMAXPROCS(0) / 2) results in 0 on systems with a single CPU core. This means that by default, child processes will not be recovered after a crash in single-core environments, which is inconsistent with the behavior on multi-core systems. Consider using a minimum value of at least 1 to ensure consistent recovery behavior across different hardware configurations.
| recoverThreshold = runtime.GOMAXPROCS(0) / 2 | |
| recoverThreshold = max(1, runtime.GOMAXPROCS(0)/2) |
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
Test plan
🤖 Generated with Claude Code