feat(prefork): Enhance prefork management with WatchMaster, CommandProducer, and Windows support#2180
Open
ReneWerner87 wants to merge 9 commits intovalyala:masterfrom
Open
feat(prefork): Enhance prefork management with WatchMaster, CommandProducer, and Windows support#2180ReneWerner87 wants to merge 9 commits intovalyala:masterfrom
ReneWerner87 wants to merge 9 commits intovalyala:masterfrom
Conversation
… callback comment
Integrate upstream's OnMasterDeath callback (replaces WatchMaster bool), os.Executable() for child command, and watchMaster as method on Prefork. Keep our OnChildSpawn, OnMasterReady, OnChildRecover callbacks and CommandProducer. Update tests accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
On Windows, os.Getppid() returns a static PID that doesn't change when the parent exits (no reparenting). Use FindProcess+Wait instead, which correctly detects parent exit. Also document why masterPID comparison works for Docker containers (master PID 1 case). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The three ListenAndServe* methods had identical child setup code (listen, set ln, watch master). Extract to listenAsChild() for cleaner code. Also add comment for the magic file descriptor number 3. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Keep upstream's (addr, certKey, certFile) signature to avoid breaking callers. Fix the doc comment to match the actual parameter order instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
There was a problem hiding this comment.
Pull request overview
Enhances prefork to reduce duplication in downstream consumers (e.g., Fiber) by exposing lifecycle hooks and customizable child command creation, while also improving child/master monitoring behavior on Windows.
Changes:
- Added optional lifecycle callbacks (
OnChildSpawn,OnMasterReady,OnChildRecover) and aCommandProducerhook for custom child command creation. - Refactored child listener setup into
listenAsChild()to deduplicate logic acrossListenAndServe*methods. - Updated master-watch logic to correctly detect master exit on Windows and fixed goroutine loop-variable capture in spawn/recovery.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
prefork/prefork.go |
Adds callbacks + CommandProducer, refactors child setup, improves Windows master monitoring, and adjusts spawn/recovery loop behavior. |
prefork/prefork_test.go |
Adds tests around logger/callback fields and CommandProducer usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Lint fixes: - Remove unused Reuseport field write in test (govet/unusedwrite) - Replace fmt.Errorf with errors.New for static errors (perfsprint) Review feedback (Copilot): - Validate CommandProducer returns a started command (nil/Process check) - Clarify ListenAndServeTLS doc: parameter order and internal forwarding - Use hermetic test binary re-exec instead of external 'go' binary - Rename misleading test to reflect what it actually asserts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We've been working on reducing code duplication between Fiber and fasthttp's prefork package. Right now Fiber maintains its own prefork implementation (~200 lines) that reimplements most of what
fasthttp/preforkalready does - process spawning, child management, the recovery loop - just to hook into a few lifecycle events (logging when a child spawns, showing a startup message after all workers are up, etc.).This PR adds a small set of optional callbacks to the
Preforkstruct that make it possible for consumers like Fiber to plug into the prefork lifecycle without copying the whole thing:OnChildSpawn(pid int) error- called when a new child is forked, so the caller can track PIDs or run hooksOnMasterReady(childPIDs []int) error- called once after all children are up, useful for startup messages or metricsOnChildRecover(pid int) error- called when a crashed child gets replaced, for logging/alertingCommandProducer(files []*os.File) (*exec.Cmd, error)- allows overriding how child commands are created, which is useful for testing (injecting dummy commands instead of re-executing the binary)All fields are optional and nil-safe - existing code doesn't need to change. The zero value works exactly as before.
Additionally:
listenAsChild()extracted to remove duplicated setup code across the threeListenAndServe*methodswatchMasternow handles Windows correctly (usingFindProcess+Waitinstead ofGetppidpolling, which doesn't detect parent exit on Windows)cmdandpidas parameters instead of capturing the loop variableYou can see the practical usage in the Fiber PR that builds on this: gofiber/fiber#4210 - it replaces Fiber's entire prefork implementation with ~50 lines that configure these callbacks. The rest is handled by this package.