Skip to content

Commit 43d7cd5

Browse files
refactor: fold catch-all registry into *worker
Drops backgroundWorkerRegistry and its registry.workers/registry.declared indirection; the per-scope lookup now resolves directly to the *worker that owns each name, and the catch-all *worker hosts all of its lazy-spawned threads in catchAllNames keyed by the runtime name. worker struct gains: - catchAllCap / catchAllMu / catchAllNames for the catch-all template (was registry.maxWorkers / registry.mu / registry.workers); - bgLazyStartMu / bgLazyStarted to gate the first-thread spawn for num=0 named declarations. backgroundWorkerThread gains runtimeName + backgroundReady so a single catch-all *worker can host multiple per-name threads, each carrying its own metric label, $_SERVER['FRANKENPHP_WORKER_NAME'] and readiness slot. setupScript trims any "m#" prefix at the PHP boundary, so module-worker metric labels stay m#-prefixed (unchanged) while PHP sees the user-facing name. ensure() picks named via lookup.byName (lazy-starting the first thread under bgLazyStartMu when num=0) or catch-all via lookup.catchAll, locks catchAllMu across the cap check + thread reservation + entry publication so concurrent callers can't observe a phantom registration on a failed allocation. User-visible behaviour preserved: per-scope isolation, per-name $_SERVER worker name, per-name metric buckets, max_threads cap on catch-alls, named pool semantics, multi-entrypoint declarations.
1 parent 19a5489 commit 43d7cd5

6 files changed

Lines changed: 186 additions & 247 deletions

background_worker_ensure_test.go

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -125,11 +125,11 @@ func TestEnsureBackgroundWorkerUndeclared(t *testing.T) {
125125
}
126126

127127
// TestEnsureBackgroundWorkerConcurrent confirms the doc claim that ensure()
128-
// is safe to call concurrently: 16 goroutines hitting the same name produce
129-
// exactly one running worker, and the registry is left in a consistent state.
130-
// Without holding registry.mu across getInactivePHPThread, a race window
131-
// could let one caller publish a registration that another caller observes
132-
// just before the first caller fails thread allocation and rolls it back.
128+
// is safe to call concurrently: 16 goroutines hitting the same lazy-named
129+
// declaration produce exactly one spawned thread. Without serialising the
130+
// lazy-start gate (bgLazyStartMu), the second caller could observe the
131+
// flag before the first caller has completed thread reservation, leaving
132+
// the worker in an inconsistent state.
133133
func TestEnsureBackgroundWorkerConcurrent(t *testing.T) {
134134
cwd, _ := os.Getwd()
135135
testDataDir := cwd + "/testdata/"
@@ -161,17 +161,12 @@ func TestEnsureBackgroundWorkerConcurrent(t *testing.T) {
161161
require.NoError(t, err, "goroutine %d", i)
162162
}
163163

164-
// Registry should hold exactly one entry for the name; the worker
165-
// itself should have exactly one attached thread.
164+
// The lazy-named declaration should resolve to its single *worker,
165+
// and exactly one thread should have been spawned by the lazy-start
166+
// path despite the 16 concurrent ensure() callers.
166167
lookup := backgroundLookups[0]
167168
require.NotNil(t, lookup)
168-
registry := lookup.resolve("bg-concurrent")
169-
require.NotNil(t, registry)
170-
registry.mu.Lock()
171-
count := len(registry.workers)
172-
w := registry.workers["bg-concurrent"]
173-
registry.mu.Unlock()
174-
assert.Equal(t, 1, count, "exactly one entry expected")
169+
w := lookup.byName["bg-concurrent"]
175170
require.NotNil(t, w)
176171
assert.Equal(t, 1, w.countThreads(), "exactly one worker thread expected")
177172
}

background_worker_pool_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -49,9 +49,9 @@ func TestBackgroundWorkerPool(t *testing.T) {
4949
}
5050

5151
// TestBackgroundWorkerMultiEntrypoint declares two named bg workers that
52-
// share the same entrypoint file. Each gets its own registry entry, so
53-
// both Init successfully (no filename-collision rejection) and both
54-
// produce sentinels.
52+
// share the same entrypoint file. Each gets its own *worker, so both Init
53+
// successfully (no filename-collision rejection) and both produce
54+
// sentinels.
5555
func TestBackgroundWorkerMultiEntrypoint(t *testing.T) {
5656
cwd, _ := os.Getwd()
5757
testDataDir := cwd + "/testdata/"

background_worker_scope_test.go

Lines changed: 13 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -49,16 +49,16 @@ func TestBackgroundWorkerSameNameDifferentScope(t *testing.T) {
4949
))
5050
t.Cleanup(Shutdown)
5151

52-
// Both lookups must exist and resolve "shared" to a registry.
52+
// Both lookups must exist and resolve "shared" to a *worker.
5353
require.NotNil(t, backgroundLookups[scopeA], "scope A lookup missing")
5454
require.NotNil(t, backgroundLookups[scopeB], "scope B lookup missing")
5555
assert.NotNil(t, backgroundLookups[scopeA].byName["shared"], "scope A must resolve 'shared'")
5656
assert.NotNil(t, backgroundLookups[scopeB].byName["shared"], "scope B must resolve 'shared'")
57-
// And they must be distinct registries (not the same pointer).
57+
// And they must be distinct *worker instances (not the same pointer).
5858
assert.NotSame(t,
5959
backgroundLookups[scopeA].byName["shared"],
6060
backgroundLookups[scopeB].byName["shared"],
61-
"each scope must own a distinct registry for the same name")
61+
"each scope must own a distinct *worker for the same name")
6262
}
6363

6464
// TestBackgroundWorkerCatchAllPerScope declares a catch-all in two
@@ -93,8 +93,8 @@ func TestBackgroundWorkerCatchAllPerScope(t *testing.T) {
9393
t.Cleanup(Shutdown)
9494

9595
// Pre-conditions: each scope's catch-all is empty.
96-
require.Empty(t, backgroundLookups[scopeA].catchAll.workers, "scope A catch-all must start empty")
97-
require.Empty(t, backgroundLookups[scopeB].catchAll.workers, "scope B catch-all must start empty")
96+
require.Empty(t, backgroundLookups[scopeA].catchAll.catchAllNames, "scope A catch-all must start empty")
97+
require.Empty(t, backgroundLookups[scopeB].catchAll.catchAllNames, "scope B catch-all must start empty")
9898

9999
// Drive ensure() through a fake "request" context tagged to scope A.
100100
// We use a request-scope tag (rather than a worker handler) because
@@ -108,31 +108,16 @@ func TestBackgroundWorkerCatchAllPerScope(t *testing.T) {
108108

109109
// The lazy-started instance must land in scope A's catch-all only.
110110
require.Eventually(t, func() bool {
111-
_, ok := backgroundLookups[scopeA].catchAll.workers["job-a"]
111+
_, ok := backgroundLookups[scopeA].catchAll.catchAllNames["job-a"]
112112
return ok
113113
}, 2*time.Second, 10*time.Millisecond, "ensure() must populate scope A's catch-all")
114-
assert.Empty(t, backgroundLookups[scopeB].catchAll.workers, "scope B catch-all must remain untouched")
115-
116-
// Verify the live workers slice has exactly one freshly-built bg
117-
// worker carrying scope A and name "job-a", and zero workers named
118-
// "job-a" carrying scope B (which would mean the wrong catch-all
119-
// got consumed).
120-
scalingMu.Lock()
121-
var jobAInA, jobAInB int
122-
for _, w := range workers {
123-
if !w.isBackgroundWorker || w.name != "job-a" {
124-
continue
125-
}
126-
switch w.backgroundScope {
127-
case scopeA:
128-
jobAInA++
129-
case scopeB:
130-
jobAInB++
131-
}
132-
}
133-
scalingMu.Unlock()
134-
assert.Equal(t, 1, jobAInA, "exactly one freshly-built bg worker must carry scope A and name 'job-a'")
135-
assert.Equal(t, 0, jobAInB, "no 'job-a' worker must carry scope B (its catch-all wasn't ensured)")
114+
assert.Empty(t, backgroundLookups[scopeB].catchAll.catchAllNames, "scope B catch-all must remain untouched")
115+
116+
// All catch-all threads attach to the scope's catch-all *worker, so
117+
// scope A's catch-all should have one thread (for "job-a") while
118+
// scope B's catch-all should have none.
119+
assert.Equal(t, 1, backgroundLookups[scopeA].catchAll.countThreads(), "scope A catch-all must host exactly one thread")
120+
assert.Equal(t, 0, backgroundLookups[scopeB].catchAll.countThreads(), "scope B catch-all must host zero threads")
136121

137122
// Wait for the worker to write its sentinel so we know the right
138123
// fixture ran (sanity check that env was inherited from scope A).

0 commit comments

Comments
 (0)