Repository navigation
Conversation
There was a problem hiding this comment.
I left one inline note on Service::stop(): the new drain path never completes local subscription cleanup, so a stopped service remains registered in Connection::$subscriptions and can be replayed on reconnect.
I verified the rest of the surface locally with:
vendor/bin/phpunit tests/Integration/Micro/ServiceTest.phpvendor/bin/phpunit tests/Integration/Microvendor/bin/phpstan analyse src --no-progress
I also reproduced the stop issue with a one-off script: after stop(), numSubscriptions() stayed unchanged instead of dropping the service subscriptions.
There was a problem hiding this comment.
The latest commit addresses my previous Service::stop() cleanup concern. I reran the micro coverage on the updated head and did not find further issues.
Verified locally with:
vendor/bin/phpunit tests/Integration/Micro/ServiceTest.phpvendor/bin/phpunit tests/Integration/Micro/GroupQueueTest.phpvendor/bin/phpunit tests/Integration/Micro/ServiceExtendedTest.phpvendor/bin/phpunit tests/Integration/Micro/StatsHandlerTest.phpvendor/bin/phpunit tests/Integration/Microvendor/bin/phpstan analyse src --no-progress
I also reran the self-stop repro that previously left service subscriptions behind; after the fix the connection drops back to only the request mux subscription as expected.
There was a problem hiding this comment.
Solid PR — the lifecycle tightening, drain-based stop, and runtime error visibility are all well-motivated. Two edge cases worth considering:
1. errorHandler can break cleanup flow
recordError() calls the user-provided errorHandler without a try/catch. If that handler throws, it can leave finishPendingStopCleanup() and stop() in an inconsistent state — e.g. the unsubscribe() fallback loop after a failed flush() never runs:
// finishPendingStopCleanup()
try {
$this->connection->flush(0.5);
} catch (\Throwable $e) {
$this->recordError($e); // if errorHandler throws here → subs never cleaned up
foreach ($subs as $sub) {
$sub->unsubscribe();
}
return;
}Consider wrapping the errorHandler invocation in recordError():
private function recordError(\Throwable $e): void
{
$this->lastError = $e;
if ($this->config->errorHandler !== null) {
try {
($this->config->errorHandler)($this, $e);
} catch (\Throwable) {
// User handler must not break service internals
}
}
}2. doneHandler fires before deferred cleanup completes
When stop() is called inside a handler (handlerDepth > 0), doneHandler is invoked immediately — but subscriptions aren't cleaned up until the handler unwinds via finishPendingStopCleanup(). This means from the doneHandler's perspective numSubscriptions() is still non-zero and the service isn't truly shut down yet. If that's intentional, a doc comment on the timing would help.
Everything else looks clean — hasResponded() guard, the subjectFor()/queueGroupFor() extractions, the Group pending-limits fix, and the test coverage are all good.
There was a problem hiding this comment.
Both concerns from my previous review are addressed in 0756d64f:
-
recordError()now wrapserrorHandlerin try/catch — a throwing user callback can no longer break cleanup or reply flow. Covered bytestThrowingErrorHandlerDoesNotBreakErrorFlow. -
doneHandleris now deferred until after cleanup —invokeDoneHandler()with adoneHandlerCalledguard ensures it fires exactly once, after subscriptions are finalized.testDoneHandlerRunsAfterDeferredStopCleanupverifiesnumSubscriptions() === 0atdoneHandlerinvocation time.
doneHandler is also wrapped in try/catch, and the invokeDoneHandler idempotency guard handles the multiple call-sites in stop() cleanly.
The rest (drain-based stop, hasResponded(), lastError(), Group pending-limits passthrough, test coverage) was already solid. LGTM.
|
I found 2 behavioral regressions here (ignoring the intentional API break):
|
Summary
stop()a graceful drain entrypoint and makereset()start a new stats windowBreaking change
ServiceConfig::$endpointbecause it did not correspond to real runtime behaviorService::lastError()/ServiceInterface::lastError()as the service-level runtime error signalRequestInterface::hasResponded()and made duplicate or impossible replies explicit failuresaddEndpoint()andaddGroup()now reject stopped servicesstop()now drains owned subscriptions instead of unsubscribing them immediately when the connection is liveDetails
reset()now clears endpoint counters, clears the current service error, and starts a new stats window timestampValidation
./vendor/bin/phpunit tests/Integration/Micro --testdox./vendor/bin/phpunit tests/Unit/Micro --testdox./vendor/bin/phpstan analyse --no-progress./vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.dist.php --dry-run --diff src/Micro/Group.php src/Micro/Request.php src/Micro/RequestInterface.php src/Micro/Service.php src/Micro/ServiceConfig.php src/Micro/ServiceInterface.php tests/Integration/Micro/ServiceTest.php docs/examples/micro/01-service.phpCloses #42