Skip to content

Commit e45e48c

Browse files
dunglasdunglas
committed
feat: detect when worker crashes or terminate normally
1 parent 2e72b50 commit e45e48c

File tree

5 files changed

+41
-44
lines changed

5 files changed

+41
-44
lines changed

‎docs/compile.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ You can now use the Go library and compile our Caddy build:
7575
```console
7676
curl -L https://github.com/dunglas/frankenphp/archive/refs/heads/main.tar.gz | tar x
7777
cd frankenphp-main/caddy/frankenphp
78-
CGO_CFLAGS=$(php-config --includes) go build
78+
CGO_CFLAGS=$(php-config --includes) CGO_LDFLAGS="$(php-config --ldflags) $(php-config --libs)" go build
7979
```
8080

8181
### Using xcaddy

‎frankenphp.c‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -709,28 +709,31 @@ int frankenphp_request_startup()
709709
int frankenphp_execute_script(char* file_name)
710710
{
711711
if (frankenphp_request_startup() == FAILURE) {
712+
free(file_name);
713+
712714
return FAILURE;
713715
}
714716

715-
int status = FAILURE;
717+
int status = SUCCESS;
716718

717719
zend_file_handle file_handle;
718720
zend_stream_init_filename(&file_handle, file_name);
721+
free(file_name);
722+
719723
file_handle.primary_script = 1;
720724

721725
zend_first_try {
722-
status = php_execute_script(&file_handle);
726+
EG(exit_status) = 0;
727+
php_execute_script(&file_handle);
728+
status = EG(exit_status);
723729
} zend_catch {
724-
/* int exit_status = EG(exit_status); */
730+
status = EG(exit_status);
725731
} zend_end_try();
726732

727733
zend_destroy_file_handle(&file_handle);
728-
free(file_name);
729734

730-
if (status >= 0) {
731-
frankenphp_clean_server_context();
732-
frankenphp_request_shutdown();
733-
}
735+
frankenphp_clean_server_context();
736+
frankenphp_request_shutdown();
734737

735738
return status;
736739
}

‎frankenphp.go‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ package frankenphp
1212
// Use PHP includes corresponding to your PHP installation by running:
1313
//
1414
// export CGO_CFLAGS=$(php-config --includes)
15-
// export CGO_LDFLAGS=$(php-config --ldflags --libs)
15+
// export CGO_LDFLAGS="$(php-config --ldflags) $(php-config --libs)"
1616
//
1717
// We also set these flags for hardening: https://github.com/docker-library/php/blob/master/8.2/bookworm/zts/Dockerfile#L57-L59
1818

@@ -124,7 +124,9 @@ type FrankenPHPContext struct {
124124
// Whether the request is already closed by us
125125
closed sync.Once
126126

127-
responseWriter http.ResponseWriter
127+
responseWriter http.ResponseWriter
128+
exitStatus C.int
129+
128130
done chan interface{}
129131
currentWorkerRequest cgo.Handle
130132
}
@@ -479,9 +481,9 @@ func go_execute_script(rh unsafe.Pointer) {
479481
panic(err)
480482
}
481483

482-
// freed in frankenphp_execute_script()
483-
cFileName := C.CString(fc.scriptFilename)
484-
if C.frankenphp_execute_script(cFileName) < 0 {
484+
// scriptFilename is freed in frankenphp_execute_script()
485+
fc.exitStatus = C.frankenphp_execute_script(C.CString(fc.scriptFilename))
486+
if fc.exitStatus < 0 {
485487
panic(ScriptExecutionError)
486488
}
487489
}
@@ -500,7 +502,10 @@ func go_ub_write(rh C.uintptr_t, cBuf *C.char, length C.int) (C.size_t, C.bool)
500502
writer = fc.responseWriter
501503
}
502504

503-
i, _ := writer.Write(unsafe.Slice((*byte)(unsafe.Pointer(cBuf)), length))
505+
i, e := writer.Write(unsafe.Slice((*byte)(unsafe.Pointer(cBuf)), length))
506+
if e != nil {
507+
fc.logger.Error("write error", zap.Error(e))
508+
}
504509

505510
if fc.responseWriter == nil {
506511
fc.logger.Info(writer.(*bytes.Buffer).String())
@@ -685,6 +690,7 @@ func ExecuteScriptCLI(script string, args []string) int {
685690
argv := make([]*C.char, argc)
686691
for i, arg := range args {
687692
argv[i] = C.CString(arg)
693+
defer C.free(unsafe.Pointer(&argv[1]))
688694
}
689695

690696
return int(C.frankenphp_execute_script_cli(cScript, argc, (**C.char)(unsafe.Pointer(&argv[0]))))

‎testdata/Caddyfile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
debug
33
frankenphp {
4-
#worker ./index.php
4+
worker ./crashing-worker.php
55
}
66
}
77

‎worker.go‎

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ package frankenphp
44
// #include "frankenphp.h"
55
import "C"
66
import (
7-
"context"
87
"errors"
98
"fmt"
109
"net/http"
@@ -56,40 +55,25 @@ func startWorkers(fileName string, nbWorkers int, env map[string]string) error {
5655
defer shutdownWG.Done()
5756
for {
5857
// Create main dummy request
59-
fc := &FrankenPHPContext{
60-
env: make(map[string]string, len(env)+1),
61-
done: make(chan interface{}),
62-
}
63-
fc.scriptFilename = absFileName
64-
fc.env["SCRIPT_FILENAME"] = absFileName
65-
for k, v := range env {
66-
fc.env[k] = v
58+
r, err := http.NewRequest(http.MethodGet, filepath.Base(absFileName), nil)
59+
if err != nil {
60+
panic(err)
6761
}
68-
69-
r, err := http.NewRequestWithContext(context.WithValue(
70-
context.Background(),
71-
contextKey,
72-
fc,
73-
), "GET", "", nil)
74-
62+
r, err = NewRequestWithContext(
63+
r,
64+
WithRequestDocumentRoot(filepath.Dir(absFileName), false),
65+
WithRequestEnv(env),
66+
)
7567
if err != nil {
76-
// TODO: this should never happen, maybe can we remove this block?
77-
m.Lock()
78-
defer m.Unlock()
79-
errs = append(errs, fmt.Errorf("workers %q: unable to create main worker request: %w", absFileName, err))
80-
81-
return
68+
panic(err)
8269
}
8370

8471
l.Debug("starting", zap.String("worker", absFileName))
8572
if err := ServeHTTP(nil, r); err != nil {
86-
m.Lock()
87-
defer m.Unlock()
88-
errs = append(errs, fmt.Errorf("workers %q: unable to start: %w", absFileName, err))
89-
90-
return
73+
panic(err)
9174
}
9275

76+
fc := r.Context().Value(contextKey).(*FrankenPHPContext)
9377
if fc.currentWorkerRequest != 0 {
9478
// Terminate the pending HTTP request handled by the worker
9579
maybeCloseContext(fc.currentWorkerRequest.Value().(*http.Request).Context().Value(contextKey).(*FrankenPHPContext))
@@ -100,7 +84,11 @@ func startWorkers(fileName string, nbWorkers int, env map[string]string) error {
10084
// TODO: make the max restart configurable
10185
if _, ok := workersRequestChans.Load(absFileName); ok {
10286
workersReadyWG.Add(1)
103-
l.Error("unexpected termination, restarting", zap.String("worker", absFileName))
87+
if fc.exitStatus == 0 {
88+
l.Info("restarting", zap.String("worker", absFileName))
89+
} else {
90+
l.Error("unexpected termination, restarting", zap.String("worker", absFileName), zap.Int("exit_status", int(fc.exitStatus)))
91+
}
10492
} else {
10593
break
10694
}

0 commit comments

Comments
 (0)