Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix - graceful shutdown #1242

Merged
merged 4 commits into from
Dec 17, 2024
+18 −30
Merged

fix - graceful shutdown #1242

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da824c5
Shuts caddy down gracefully.
Alliballibaba2 Dec 12, 2024
6c0d33f
Moves isRunning to the very end.
Alliballibaba2 Dec 12, 2024
ccf0b33
Changes check to Exiting().
Alliballibaba2 Dec 13, 2024
2d4a115
Merge branch 'main' into fix/graceful-shutdown
Alliballibaba2 Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 10 additions & 29 deletions caddy/caddy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,22 +40,8 @@ func init() {
httpcaddyfile.RegisterDirectiveOrder("php_server", "before", "file_server")
}

type mainPHPinterpreterKeyType int

var mainPHPInterpreterKey mainPHPinterpreterKeyType

var phpInterpreter = caddy.NewUsagePool()

var metrics = frankenphp.NewPrometheusMetrics(prometheus.DefaultRegisterer)

type phpInterpreterDestructor struct{}

func (phpInterpreterDestructor) Destruct() error {
frankenphp.Shutdown()

return nil
}

type workerConfig struct {
// FileName sets the path to the worker script.
FileName string `json:"file_name,omitempty"`
Expand Down Expand Up @@ -91,29 +77,24 @@ func (f *FrankenPHPApp) Start() error {
opts = append(opts, frankenphp.WithWorkers(repl.ReplaceKnown(w.FileName, ""), w.Num, w.Env, w.Watch))
}

_, loaded, err := phpInterpreter.LoadOrNew(mainPHPInterpreterKey, func() (caddy.Destructor, error) {
if err := frankenphp.Init(opts...); err != nil {
return nil, err
}

return phpInterpreterDestructor{}, nil
})
if err != nil {
frankenphp.Shutdown()
if err := frankenphp.Init(opts...); err != nil {
return err
}

if loaded {
frankenphp.Shutdown()
if err := frankenphp.Init(opts...); err != nil {
return err
}
}

return nil
}

func (f *FrankenPHPApp) Stop() error {
caddy.Log().Info("FrankenPHP stopped 🐘")

// attempt a graceful shutdown if caddy is exiting
// note: Exiting() is currently marked as 'experimental'
// https://github.com/caddyserver/caddy/blob/e76405d55058b0a3e5ba222b44b5ef00516116aa/caddy.go#L810
if caddy.Exiting() {
frankenphp.Shutdown()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot work because in case of reload, Caddy starts the new module before stopping the old one.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I think I get it now. There's an experimental caddy.Exiting() api that we could maybe use instead.

Is there also a way to reload Caddy outside of tests? Running curl "http://localhost:2019/load" via the admin API also doesn't work on main.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nvm I realized you have to actually patch the whole config for a reload

curl "http://localhost:2019/config/apps/frankenphp" -X PATCH --header "Content-Type: application/json" -d '{"workers"
:[{"file_name":"/go/src/app/testdata/index.php","watch":["./**/*.{php,yaml,yml,twig,env}"]}]}' -H "Cache-Control: must-revalidate"

This works now with the Exiting() check if you're fine with using an experimental api. Otherwise I'll close this PR.

Copy link
Contributor

@francislavoie francislavoie Dec 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use --force if you're using caddy reload or equivalent, which is the same as passing the Cache-Control: must-revalidate header in the API to reload even if the config text has not changed.

}

// reset configuration so it doesn't bleed into later tests
f.Workers = nil
f.NumThreads = 0
Expand Down
9 changes: 8 additions & 1 deletion frankenphp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ var (
ScriptExecutionError = errors.New("error during PHP script execution")

requestChan chan *http.Request
isRunning bool
done chan struct{}
shutdownWG sync.WaitGroup

Expand Down Expand Up @@ -277,9 +278,10 @@ func calculateMaxThreads(opt *opt) error {

// Init starts the PHP runtime and the configured workers.
func Init(options ...Option) error {
if requestChan != nil {
if isRunning {
return AlreadyStartedError
}
isRunning = true

// Ignore all SIGPIPE signals to prevent weird issues with systemd: https://github.com/dunglas/frankenphp/issues/1020
// Docker/Moby has a similar hack: https://github.com/moby/moby/blob/d828b032a87606ae34267e349bf7f7ccb1f6495a/cmd/dockerd/docker.go#L87-L90
Expand Down Expand Up @@ -362,6 +364,10 @@ func Init(options ...Option) error {

// Shutdown stops the workers and the PHP runtime.
func Shutdown() {
if !isRunning {
return
}

drainWorkers()
drainThreads()
metrics.Shutdown()
Expand All @@ -373,6 +379,7 @@ func Shutdown() {
}

logger.Debug("FrankenPHP shut down")
isRunning = false
}

//export go_shutdown
Expand Down
Loading