Skip to content

Commit 2ca5ffc

Browse files
authored
chore: simplify and modernize for Go 1.27 (#2627)
Follow-up to the Go 1.27 upgrade in a411024, applying what the new release makes available. ## Modernizers `go fix` gained `slicesbackward`, `embedlit` and `unsafefuncs`, and renamed `waitgroup` to `waitgroupgo`. Applied across both modules, with two exceptions: - The extgen parsers are left alone: `embedlit` rewrites their composite literals into a shape that reads worse than the original. - `go fix` skips files that import `"C"`, so the root package never got the transformations applied everywhere else. Those are done by hand in a separate commit. ## Standard library - `splitRemoteAddr` uses `strings.CutLast`, new in 1.27. Its not-found return is exactly what the old `else` branch assigned, so the branch disappears. - The manual `unsafe.Pointer`/`uintptr` arithmetic in `types.go` and `frankenphp.go` becomes `unsafe.Add`. The old form is only valid inside a single expression and silently breaks if a later refactor splits the line. - `errors.As` becomes `errors.AsType`, dropping the throwaway target values. - Eleven copies of the register-or-panic block in `metrics.go` collapse into one `mustRegister` helper. ## Tests `internal/state` moves to `testing/synctest`. `synctest.Wait` returns once every subscriber goroutine is durably blocked, so the subscriber count is exact rather than polled for up to a second. This also makes the `WaitForStateWithTimeout` give-up path worth covering, since the bubble's fake clock fires its one second timeout instantly. The other timing tests stay as they are. `regularRequestChan` is a package global that live PHP thread goroutines receive from, so a bubbled sender can have its send stolen by an out-of-bubble thread, and PHP threads are C threads, which never count as durably blocked. Bubbling them would buy flakiness, not determinism. A new test uses the `goroutineleak` profile, GA in 1.27, to catch a thread, scaling ticker or watcher goroutine that outlives `Shutdown` with nothing left to wake it. Currently reports zero leaks across a full `Init`/`Shutdown` cycle. `/debug/pprof/goroutineleak` needs no code to expose: `net/http/pprof.Index` dispatches profiles by name, so it is already live on the admin endpoint. Verified against a running server. It reports one leaked goroutine there, but the same one appears with no `php_server` configured at all, so it is Caddy/runtime baseline rather than ours. ## Drive-by `internal/state` had a bare `import "C"` with no C preamble and no calls into C, which forced a pure Go package through cgo. Removed. ## No action needed `encoding/json/v2` backing `encoding/json` and the size-specialized allocator are both in the 1.27 baseline and active automatically. The stricter jsonv2 defaults ship only with an explicit `encoding/json/v2` import, so v1 semantics are unchanged: duplicate keys still accepted, invalid UTF-8 still replaced rather than rejected.
1 parent a411024 commit 2ca5ffc

24 files changed

Lines changed: 164 additions & 128 deletions

caddy/caddy_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -903,7 +903,7 @@ func TestPhpServerWorkerMatchPoolCount(t *testing.T) {
903903
require.NoError(t, err, "failed to read metrics")
904904

905905
var pools []string
906-
for _, line := range strings.Split(metrics.String(), "\n") {
906+
for line := range strings.SplitSeq(metrics.String(), "\n") {
907907
if !strings.HasPrefix(line, "frankenphp_total_workers{worker=") {
908908
continue
909909
}
@@ -1830,7 +1830,7 @@ func TestOpcacheReset(t *testing.T) {
18301830
wg := sync.WaitGroup{}
18311831
numRequests := 500
18321832
wg.Add(numRequests)
1833-
for i := 0; i < numRequests; i++ {
1833+
for i := range numRequests {
18341834

18351835
// introduce a delay every 10 requests
18361836
if i%10 == 0 {
@@ -2138,7 +2138,7 @@ func TestSymlinkWorkerBehavior(t *testing.T) {
21382138
`, "caddyfile")
21392139

21402140
// Make multiple requests - each should increment the counter
2141-
for i := 0; i < 5; i++ {
2141+
for i := range 5 {
21422142
tester.AssertGetResponse("http://localhost:"+testPort+"/index.php", http.StatusOK, fmt.Sprintf("Request: %d\n", i))
21432143
}
21442144
})

caddy/config_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ func TestCreateUniqueWorkerNames(t *testing.T) {
254254
filename := "../testdata/worker-with-env.php"
255255
absFileName, _ := filepath.Abs(filename)
256256
names := make([]string, 6)
257-
for i := 0; i < 3; i++ {
257+
for i := range 3 {
258258
names[i] = app.createUniqueWorkerName(workerConfig{
259259
FileName: filename,
260260
Name: "custom-worker-name",

caddy/go.mod

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,6 @@ require (
1919
github.com/stretchr/testify v1.11.1
2020
)
2121

22-
require github.com/smallstep/go-attestation v0.4.9 // indirect
23-
2422
require (
2523
cel.dev/expr v0.25.2 // indirect
2624
cloud.google.com/go/auth v0.20.0 // indirect
@@ -141,6 +139,7 @@ require (
141139
github.com/slackhq/nebula v1.10.3 // indirect
142140
github.com/smallstep/certificates v0.30.2 // indirect
143141
github.com/smallstep/cli-utils v0.12.2 // indirect
142+
github.com/smallstep/go-attestation v0.4.9 // indirect
144143
github.com/smallstep/linkedca v0.25.0 // indirect
145144
github.com/smallstep/nosql v0.8.0 // indirect
146145
github.com/smallstep/pkcs7 v0.2.1 // indirect

caddy/module.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c
227227

228228
err := f.server.ServeHTTP(w, r, opts...)
229229

230-
if err != nil && !errors.As(err, &frankenphp.ErrRejected{}) {
230+
if _, rejected := errors.AsType[frankenphp.ErrRejected](err); err != nil && !rejected {
231231
return caddyhttp.Error(http.StatusInternalServerError, err)
232232
}
233233

cgi.go

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ func splitPos(path string, splitPath []string) int {
253253

254254
for i := 0; i <= pathLen-splitLen; i++ {
255255
match := true
256-
for j := 0; j < splitLen; j++ {
256+
for j := range splitLen {
257257
c := path[i+j]
258258
if c >= utf8.RuneSelf {
259259
match = false
@@ -363,12 +363,8 @@ func splitRemoteAddr(remoteAddr string) (ip, port string) {
363363
return host, p
364364
}
365365

366-
if idx := strings.LastIndex(remoteAddr, ":"); idx > -1 {
367-
ip = remoteAddr[:idx]
368-
port = remoteAddr[idx+1:]
369-
} else {
370-
ip = remoteAddr
371-
}
366+
// CutLast yields (remoteAddr, "") when there is no colon, i.e. no port.
367+
ip, port, _ = strings.CutLast(remoteAddr, ":")
372368

373369
if len(ip) >= 2 && ip[0] == '[' && ip[len(ip)-1] == ']' {
374370
ip = ip[1 : len(ip)-1]

cli_test.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,7 @@ func TestExecuteScriptCLI(t *testing.T) {
2121
stdoutStderr, err := cmd.CombinedOutput()
2222
assert.Error(t, err)
2323

24-
var exitError *exec.ExitError
25-
if errors.As(err, &exitError) {
24+
if exitError, ok := errors.AsType[*exec.ExitError](err); ok {
2625
assert.Equal(t, 3, exitError.ExitCode())
2726
}
2827

@@ -60,8 +59,7 @@ func TestExecuteCLIPHPInfo(t *testing.T) {
6059
if frankenphp.Version().VersionID < 80600 {
6160
assert.Error(t, err)
6261

63-
var exitError *exec.ExitError
64-
if errors.As(err, &exitError) {
62+
if exitError, ok := errors.AsType[*exec.ExitError](err); ok {
6563
assert.Equal(t, 1, exitError.ExitCode())
6664
}
6765

context.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -236,8 +236,8 @@ func (fc *frankenPHPContext) reject(err error) {
236236
return
237237
}
238238

239-
re := &ErrRejected{}
240-
if !errors.As(err, re) {
239+
re, ok := errors.AsType[ErrRejected](err)
240+
if !ok {
241241
// Should never happen
242242
panic("only instance of ErrRejected can be passed to reject")
243243
}

frankenphp.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -320,7 +320,7 @@ func Init(options ...Option) error {
320320
}
321321

322322
regularThreads = make([]*phpThread, 0, opt.numThreads-workerThreadCount)
323-
for i := 0; i < opt.numThreads-workerThreadCount; i++ {
323+
for range opt.numThreads - workerThreadCount {
324324
convertToRegularThread(getInactivePHPThread())
325325
}
326326

@@ -533,7 +533,7 @@ func splitRawHeader(rawHeader *C.char, length int) (string, string) {
533533
}
534534

535535
// anything left is the header value
536-
valuePtr := (*C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(rawHeader)) + uintptr(j)))
536+
valuePtr := (*C.char)(unsafe.Add(unsafe.Pointer(rawHeader), j))
537537
headerValue := C.GoStringN(valuePtr, C.int(length-j))
538538

539539
return headerKey, headerValue

frankenphp_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ func runTest(t *testing.T, test func(func(http.ResponseWriter, *http.Request), *
8787
assert.NoError(t, err)
8888

8989
err = frankenphp.ServeHTTP(w, req)
90-
if err != nil && !errors.As(err, &frankenphp.ErrRejected{}) {
90+
if _, rejected := errors.AsType[frankenphp.ErrRejected](err); err != nil && !rejected {
9191
assert.Fail(t, fmt.Sprintf("Received unexpected error:\n%+v", err))
9292
}
9393
}

goroutineleak_test.go

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
package frankenphp
2+
3+
import (
4+
"bytes"
5+
"net/http/httptest"
6+
"runtime/pprof"
7+
"testing"
8+
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
// leakedGoroutines runs a leak-detection GC cycle and returns how many goroutines
14+
// it found blocked forever on a channel or mutex that no running goroutine can
15+
// still reach, along with their stacks.
16+
//
17+
// Profile.Count only reports the result of the previous detection cycle, so the
18+
// profile has to be written first even when only the count is wanted.
19+
func leakedGoroutines(t *testing.T) (int, string) {
20+
t.Helper()
21+
22+
profile := pprof.Lookup("goroutineleak")
23+
require.NotNil(t, profile, "the goroutineleak profile is only available since Go 1.27")
24+
25+
var stacks bytes.Buffer
26+
require.NoError(t, profile.WriteTo(&stacks, 1))
27+
28+
return profile.Count(), stacks.String()
29+
}
30+
31+
// TestNoGoroutinesAreLeakedByAFullServerLifecycle boots PHP threads, serves requests
32+
// through a worker and a regular thread, then shuts everything down.
33+
//
34+
// Shutdown has to unblock every goroutine it started, so a goroutine still parked on
35+
// an unreachable channel afterwards means a thread, a scaling ticker or a watcher
36+
// outlived Shutdown with nothing left to wake it. The count is compared against a
37+
// baseline rather than against zero: tests share a process, so earlier tests may have
38+
// left leaks of their own behind.
39+
func TestNoGoroutinesAreLeakedByAFullServerLifecycle(t *testing.T) {
40+
before, _ := leakedGoroutines(t)
41+
42+
require.NoError(t, Init(
43+
WithNumThreads(2),
44+
WithMaxThreads(4),
45+
WithWorkers("worker", testDataPath+"/index.php", 1, WithWorkerMaxFailures(0)),
46+
))
47+
48+
for range 5 {
49+
r := httptest.NewRequest("GET", "http://localhost/index.php", nil)
50+
req, err := NewRequestWithContext(r, WithRequestDocumentRoot(testDataPath, false))
51+
require.NoError(t, err)
52+
require.NoError(t, ServeHTTP(httptest.NewRecorder(), req))
53+
}
54+
55+
Shutdown()
56+
57+
after, stacks := leakedGoroutines(t)
58+
t.Logf("leaked goroutines: %d before, %d after", before, after)
59+
60+
assert.LessOrEqual(t, after, before, "goroutines leaked across an Init/Shutdown cycle:\n%s", stacks)
61+
}

0 commit comments

Comments
 (0)