feat: amqp swoole #47
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AMQPSwoole
participant AMQPSwooleConnection
participant AMQPChannel
Caller->>AMQPSwoole: withChannel(callback)
alt Channel exists
AMQPSwoole->>AMQPChannel: Use existing channel
else Channel does not exist
AMQPSwoole->>AMQPSwooleConnection: Create new connection
AMQPSwooleConnection->>AMQPChannel: Create new channel
AMQPSwoole->>AMQPChannel: Store and use new channel
end
AMQPSwoole->>Caller: Execute callback(channel)
alt Exception occurs
AMQPSwoole->>AMQPChannel: Reset channel
AMQPSwoole->>AMQPSwooleConnection: Create new connection
AMQPSwooleConnection->>AMQPChannel: Create new channel
AMQPSwoole->>Caller: Retry callback(channel)
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
composer.json (1)
26-31: Prefer HTTPS over SSH for public composer repositoriesUsing an
ssh://style URL ([email protected]:…) forces every consumer (including CI runners) to have SSH keys configured.
Switching to HTTPS (https://github.com/appwrite-labs/php-amqplib.git) makes the package consumable out-of-the-box and avoids flaky builds when the SSH agent isn’t available.- "url": "[email protected]:appwrite-labs/php-amqplib.git" + "url": "https://github.com/appwrite-labs/php-amqplib.git"src/Queue/Broker/AMQPSwoole.php (1)
16-35: Channel factory duplicates parent logic – introduce a shared helper
$createChannelis copy-pasted from the parent class with only the connection class changed. This violates DRY and risks diverging behaviour.Possible directions:
- Move the factory into a protected
createConnection(): AbstractConnectionin the parent and only override that.- Accept a
callable $connectionFactoryvia constructor.Keeps reconnection flow in one place and simplifies future fixes.
src/Queue/Broker/AMQP.php (2)
22-26: Visibility relaxed – ensure invariants are documentedExposing
$exchangeArguments,$queueArguments, … asprotectedempowers subclasses but removes encapsulation guarantees.
Add PHPDoc notes describing the expected shapes (array<string,string>etc.) and whether mutation after initialisation is allowed.
185-221: withChannel logic duplicated in subclass – candidate for Template MethodGiven the near-identical method in
AMQPSwoole, extract the common reconnect wrapper into the parent and delegate only connection creation to a small overridable hook (protected function createChannel(): AMQPChannel). Reduces maintenance overhead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
composer.json(1 hunks)src/Queue/Broker/AMQP.php(2 hunks)src/Queue/Broker/AMQPSwoole.php(1 hunks)
🔇 Additional comments (1)
src/Queue/Broker/AMQP.php (1)
38-47: Constructor parameters madereadonly– 👍Switching to
readonlyensures connection details remain immutable after construction, preventing subtle race conditions when objects are reused.
| "require": { | ||
| "php": ">=8.3", | ||
| "php-amqplib/php-amqplib": "^3.7", | ||
| "appwrite-labs/php-amqplib": "master-dev", | ||
| "utopia-php/cli": "0.15.*", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Unpinned “master-dev” constraint is brittle
"appwrite-labs/php-amqplib": "master-dev" points to a moving target. Any force-push on that branch can silently break downstream builds.
Pin to a tag or specific commit and optionally allow caret/tilde ranges for controlled updates:
- "appwrite-labs/php-amqplib": "master-dev",
+ "appwrite-labs/php-amqplib": "1.0.0", // or an exact commit hashFollow-up: If no stable tag exists yet, consider adding "minimum-stability": "dev" with an explicit "prefer-stable": true to keep Composer satisfied.
🏁 Script executed:
#!/bin/bash
# Locate the php-amqplib entry and surrounding context
grep -R '"appwrite-labs/php-amqplib"' -n composer.json -A3 -B2
# Check for existing stability settings
grep -R '"minimum-stability"' -n composer.json
grep -R '"prefer-stable"' -n composer.jsonLength of output: 424
Pin unstable dependency instead of tracking master-dev
Your composer.json is pulling a moving target for the AMQP library:
- Location: composer.json (line 34)
- Current requirement:
"appwrite-labs/php-amqplib": "master-dev",
This can lead to silent breakages whenever that branch is force-pushed. Pin to a released version or exact commit:
- "appwrite-labs/php-amqplib": "master-dev",
+ "appwrite-labs/php-amqplib": "1.0.0", // or "dev-master as 1.0.0" / specific commit hashYou already have
"minimum-stability": "stable"so the -dev suffix on your requirement will still install the branch. No change to stability settings is required unless you intend to broadly allow dev-level packages—in which case add:
"minimum-stability": "dev",
"prefer-stable": true🤖 Prompt for AI Agents
In composer.json around lines 32 to 35, the dependency
"appwrite-labs/php-amqplib" is set to "master-dev", which is a moving target and
can cause silent breakages. Replace "master-dev" with a specific released
version tag or an exact commit hash to pin the dependency. Since
"minimum-stability" is already set to "stable", no changes are needed there
unless you want to allow dev packages more broadly, in which case add
"minimum-stability": "dev" and "prefer-stable": true.
| if (!$this->channel) { | ||
| $this->channel = $createChannel(); | ||
| } | ||
|
|
There was a problem hiding this comment.
$this->channel is shared across Swoole coroutines – risk of data races
With Swoole each coroutine can resume at any point; sharing a single AMQPChannel instance can lead to:
• Unexpected frame interleaving
• “Channel closed” exceptions when one coroutine finishes/acks while another consumes
Consider storing the channel in coroutine-local storage (Swoole\Coroutine::getCid() keyed array) or creating a new channel per withChannel invocation.
🤖 Prompt for AI Agents
In src/Queue/Broker/AMQPSwoole.php around lines 37 to 40, the current code
shares a single AMQPChannel instance across all Swoole coroutines, which can
cause data races and channel errors. To fix this, modify the code to either
store the channel in a coroutine-local storage keyed by
Swoole\Coroutine::getCid() or create a new AMQPChannel instance each time
withChannel is invoked, ensuring each coroutine uses its own separate channel
instance.
| try { | ||
| $callback($this->channel); | ||
| } catch (\Throwable) { | ||
| // createChannel() might throw, in that case set the channel to `null` first. | ||
| $this->channel = null; | ||
| // try creating a new connection once, if this still fails, throw the error | ||
| $this->channel = $createChannel(); | ||
| $callback($this->channel); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
First exception is lost during reconnect – hampers debugging
The catch block discards the original Throwable. If the reconnection also fails, the second exception bubbles up and the root cause is gone.
Capture and chain:
- } catch (\Throwable) {
+ } catch (\Throwable $first) {
…
- $this->channel = $createChannel();
- $callback($this->channel);
+ try {
+ $this->channel = $createChannel();
+ $callback($this->channel);
+ } catch (\Throwable $second) {
+ $second->addPrevious($first); // or include it in a new RuntimeException
+ throw $second;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Queue/Broker/AMQPSwoole.php around lines 41 to 49, the catch block
discards the original Throwable, causing loss of the root cause if the
reconnection also fails. Modify the catch block to capture the first exception,
then if the reconnection fails, throw a new exception that chains or includes
the original one to preserve the root cause for debugging.
Summary by CodeRabbit
New Features
Chores