refactor: replace static resource callbacks with DI Container#74
Open
ChiragAgg5k wants to merge 4 commits intomainfrom
Open
refactor: replace static resource callbacks with DI Container#74ChiragAgg5k wants to merge 4 commits intomainfrom
ChiragAgg5k wants to merge 4 commits intomainfrom
Conversation
Migrate Server from static resource management to Utopia DI Container. Introduce workerContainer for worker-scoped resources while keeping the global container for shared state.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds a runtime dependency Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/Queue/Server.php (2)
330-334: Silent exception swallowing in workerStop hooks.Exceptions from
workerStophooks are silently caught and ignored. This could mask important errors during shutdown. Consider at minimum logging the exception.♻️ Proposed improvement: Log or record the exception
try { $hook->getAction()(...$this->getArguments($this->workerContainer, $hook)); } catch (Throwable $e) { + // Consider logging: error_log("workerStop hook failed: " . $e->getMessage()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Queue/Server.php` around lines 330 - 334, The workerStop hook loop currently swallows all exceptions (catch (Throwable $e) { }) which hides shutdown errors; update the catch to log the exception from $hook->getAction() using an available logger (e.g. $this->logger->error) or a safe fallback (error_log) and include $e->getMessage() and $e->getTraceAsString(); ensure you still continue running other hooks and keep the invocation via $this->getArguments($this->workerContainer, $hook) unchanged.
444-447: Validators accumulate in the container and may conflict.Each call to
validate()with a callable validator stores it in the container with key_validator:{key}. This has two concerns:
- Accumulation: Validator instances persist in the container, potentially growing unbounded over many job invocations.
- Key collision: If different hooks use the same param key with different validators, they'll share/overwrite the same container key.
Consider using a more unique key (include hook identity) or not storing validators in the container at all since they're transient.
♻️ Proposed fix: Invoke validator directly without storing
if (\is_callable($validator)) { - $validatorKey = '_validator:' . $key; - $container->set($validatorKey, $validator, $param['injections']); - $validator = $container->get($validatorKey); + // Resolve injections and call the validator factory + $injectionValues = array_map(fn($name) => $container->get($name), $param['injections']); + $validator = $validator(...$injectionValues); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Queue/Server.php` around lines 444 - 447, The validator instances are being stored in the container under a shared key ($validatorKey = '_validator:' . $key) via $container->set / $container->get which causes accumulation and collisions; instead either invoke the callable validator directly (call $validator with the param and injections) without storing it in the container, or if you must register it, generate a truly unique key that includes the hook identity (e.g. hook name/id or spl_object_hash($validator)) so it won't collide, and ensure you remove/unset the key after invocation to avoid retention; update the code paths around validate(), $validatorKey, $container->set and $container->get to implement one of these approaches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Queue/Server.php`:
- Around line 236-237: The shared container is being mutated with
$this->getContainer()->set('message', ...) which can cause race conditions when
jobs run concurrently; instead, avoid writing to the global/shared container —
create an isolated per-job context (e.g., clone the container or create a
scoped/child container) and call set('message', ...) on that per-job container,
or store the message in coroutine-local storage and pass that scoped
container/context into the job processing flow so getContainer() is not mutated
globally; update the code paths that consume message to use the per-job
container/context rather than the shared $this->getContainer().
- Around line 311-319: The error and message are being stored on the shared
container inside the closure (see the anonymous function parameters,
getContainer()->set('error', fn () => $th) and set('message', ...)) which
creates a race when multiple jobs run concurrently; instead avoid mutating
shared container state by passing the error and message directly to each hook
invocation or by creating a per-hook scoped container before calling
$hook->getAction(). Concretely, update the loop that iterates $this->errorHooks
to either call $hook->getAction() with explicit parameters (e.g. append $message
and $th to the arguments returned by getArguments) or build a short-lived
cloned/scoped container containing only 'error' and 'message' for that single
hook call and do not write to the shared container; change getArguments or
call-site accordingly and remove the shared getContainer()->set(...) usage to
eliminate the race.
- Line 66: The $coroutines property on the Server class is never switched to
true so the coroutine branch in getContainer() is dead; add a public setter
(e.g., enableCoroutines() or setCoroutines(bool $enable)) on the class that sets
the protected bool $coroutines property and document intent, and update any
initialization path to call that setter when coroutine mode should be active;
reference the Server::$coroutines property and the Server::getContainer() method
so the coroutine check can become reachable.
- Around line 108-115: The coroutine context key WORKER_CONTAINER_CONTEXT_KEY
used by getContainer() is never set, so per-coroutine isolation never occurs;
populate \Swoole\Coroutine::getContext()[self::WORKER_CONTAINER_CONTEXT_KEY]
when a coroutine begins processing a job (e.g. in the job message consumer
callback or the worker loop that spawns coroutines) by creating a new Container
instance seeded from $this->workerContainer ?? $this->container and assigning it
into the coroutine context so getContainer() will return the per-coroutine
Container instead of the shared one.
---
Nitpick comments:
In `@src/Queue/Server.php`:
- Around line 330-334: The workerStop hook loop currently swallows all
exceptions (catch (Throwable $e) { }) which hides shutdown errors; update the
catch to log the exception from $hook->getAction() using an available logger
(e.g. $this->logger->error) or a safe fallback (error_log) and include
$e->getMessage() and $e->getTraceAsString(); ensure you still continue running
other hooks and keep the invocation via
$this->getArguments($this->workerContainer, $hook) unchanged.
- Around line 444-447: The validator instances are being stored in the container
under a shared key ($validatorKey = '_validator:' . $key) via $container->set /
$container->get which causes accumulation and collisions; instead either invoke
the callable validator directly (call $validator with the param and injections)
without storing it in the container, or if you must register it, generate a
truly unique key that includes the hook identity (e.g. hook name/id or
spl_object_hash($validator)) so it won't collide, and ensure you remove/unset
the key after invocation to avoid retention; update the code paths around
validate(), $validatorKey, $container->set and $container->get to implement one
of these approaches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53b554fa-b958-4789-813f-8534cbfffe92
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
composer.jsonsrc/Queue/Server.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Queue/Server.php (1)
67-67:⚠️ Potential issue | 🟠 MajorCoroutine mode cannot be enabled from this class API.
$coroutinesis alwaysfalsehere, so coroutine container logic is effectively dead unless something outside this class mutates the property.🔧 Suggested fix
protected bool $coroutines = false; protected Container $container; protected ?Container $workerContainer = null; + +public function setCoroutines(bool $enabled): self +{ + $this->coroutines = $enabled; + return $this; +}Also applies to: 218-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Queue/Server.php` at line 67, The Server class hardcodes protected bool $coroutines = false so coroutine-mode code paths never run; change the class API to allow enabling coroutines (either accept a bool $coroutines parameter in the constructor and assign it to $this->coroutines, or add a public setCoroutines(bool $enabled) method and use that before initialization), update code that initializes the coroutine container to read $this->coroutines (e.g., in the constructor or init method that references the coroutine logic around the same area as the code at lines 218-220), and ensure any callers (factory/instantiation sites) pass the desired flag so the coroutine container logic is reachable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Queue/Server.php`:
- Around line 109-115: The $workerContainer property is never initialized so
non-coroutine code falls back to the root container and pollutes global state;
initialize and assign a worker-scoped container instance to
$this->workerContainer before any worker-scoped writes occur (e.g., at worker
startup) and have getContainer() return that worker container when available
(preserve existing coroutine behavior using WORKER_CONTAINER_CONTEXT_KEY).
Concretely, when a worker is created or about to perform worker-scoped
operations, create/clone a child container and assign it to
$this->workerContainer so subsequent calls to getContainer() (and any write
paths noted in the review) use the worker-scoped container instead of the root
container.
---
Duplicate comments:
In `@src/Queue/Server.php`:
- Line 67: The Server class hardcodes protected bool $coroutines = false so
coroutine-mode code paths never run; change the class API to allow enabling
coroutines (either accept a bool $coroutines parameter in the constructor and
assign it to $this->coroutines, or add a public setCoroutines(bool $enabled)
method and use that before initialization), update code that initializes the
coroutine container to read $this->coroutines (e.g., in the constructor or init
method that references the coroutine logic around the same area as the code at
lines 218-220), and ensure any callers (factory/instantiation sites) pass the
desired flag so the coroutine container logic is reachable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce42f7d2-1ba4-486a-a807-0eb658c00071
📒 Files selected for processing (1)
src/Queue/Server.php
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Queue/Server.php`:
- Around line 125-132: The workerContainer property is never initialized so
getContainer() falls back to the root container; in the workerStart callback
initialize a per-worker container (assign to $this->workerContainer) and seed it
with the worker-scoped entries (workerId, message, error) or copy/child the root
$this->container as appropriate, and in workerStop unset/cleanup
$this->workerContainer so it no longer persists; when coroutines are enabled
continue to use WORKER_CONTAINER_CONTEXT_KEY for coroutine-local access, but
ensure workerStart populates $this->workerContainer and workerStop clears it to
restore proper worker-scoped isolation.
- Around line 234-236: The container is being set in start() behind the false
condition Coroutine::getCid() !== -1 so WORKER_CONTAINER_CONTEXT_KEY is never
created for coroutines; move the container initialization into the message
consume callback (the function that runs per-message) so that inside the
callback where Coroutine::getCid() will be a coroutine id you set
Coroutine::getContext()[self::WORKER_CONTAINER_CONTEXT_KEY] = new
Container($this->container) before processing the message; update getContainer()
usage to rely on this per-coroutine context and keep the fallback to
$this->container only if the context key is absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 603e2a9c-e980-48dd-abda-cd778fbf6dda
📒 Files selected for processing (1)
src/Queue/Server.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
$resourcesCallbacksand$resourcesarray with Utopia DIContainerfor dependency injectionutopia-php/dias an explicit dependencyworkerContainer(child of globalcontainer) for worker-scoped resources (workerId,message,error)container, worker-specific resources onworkerContainergetContainer()method with Swoole coroutine context supportContainerexplicitly togetArguments()andvalidate()instead of relying on static stateTest plan
Summary by CodeRabbit
Chores
Refactor