Replace basic_get polling with basic_consume push for AMQP consumers (cron path)#213
Conversation
Magento already uses basic_consume (push-based) when running a consumer in daemon mode via Queue::subscribe(). However, when --max-messages is set — the path taken by every cron-spawned consumer via ConsumersRunner — CallbackInvoker falls into a dequeue() loop that calls basic_get() once per message, requiring one full network round-trip per message. RabbitMQ explicitly discourages basic_get for continuous consumption. This change adds Queue::subscribeWithLimit(), structurally identical to the existing Queue::subscribe() but cancelling the consumer via basic_cancel() once $maxMessages have been processed. CallbackInvoker now routes AMQP queues with a message limit through this method, making the cron path consistent with the CLI daemon path. The dequeue() polling loop is preserved unchanged for MySQL and STOMP backends. The consumers_wait_for_messages config is respected: value 1 blocks indefinitely (matching original behaviour), value 0 uses a 1-second idle timeout to exit promptly on an empty queue. Benchmark (5,000 messages, Docker/local RabbitMQ ~0.1ms RTT): basic_get before: ~0.72s (~6,900 msg/s) basic_consume after: ~0.30s (~16,500 msg/s) — 2.4x faster At a typical production RTT of 2ms the improvement is ~100x: 5,000 x 2ms = 10s before vs ~50 round-trips x 2ms = 0.1s after.
|
Hi @gowrizrh -- I thought of you given your AMQP experience. Any chance you can review this? Thanks @thebraziliandeveloper for the PR |
Pardon that -- I changed the PR target and that brought some other work along. I updated the target branch to fix that. This PR is isolated to the single commit now. Thank you |
gowrizrh
left a comment
There was a problem hiding this comment.
This is a neat change, thanks. I've approved it
|
Thanks @gowrizrh I see some failing checks, should I fix them so the team can merge the PR? |
- Expand empty closures `function () {}` to multi-line form to satisfy
the Magento2 coding standard (closing brace must be on its own line)
in QueueTest.php and CallbackInvokerTest.php
- Update TRemoteService.txt fixture to use nullable type `?int $typeId`
matching the TRepositoryInterface source; the old fixture used `int`
which caused RemoteServiceGeneratorTest::testGenerate mage-os#1 to fail
a660746 to
73c7bb6
Compare
|
Thanks @thebraziliandeveloper , I reran PR checks. Can you look at the AMQP unit test and integration test errors? |
|
@rhoerr will check this week |
Description
Magento already uses
basic_consume(push-based) when running a consumer in daemon mode from the CLI —Consumer::process()callsQueue::subscribe()directly when no message limit is set, andQueue::subscribe()has always usedbasic_consume+basic_qosprefetch:However, when
--max-messagesis set — which is the path taken by every cron-spawned consumer viaConsumersRunner— execution falls intoCallbackInvoker::invoke(), which uses adequeue()loop.dequeue()callsbasic_get()once per message: a synchronous request/response that requires one full network round-trip to RabbitMQ per message.This means the same consumer that efficiently subscribes when run manually in the CLI degrades to sequential polling the moment cron spawns it with
--max-messages=10000. RabbitMQ's own documentation explicitly discourages this pattern:This PR makes the cron path consistent with the CLI daemon path. It adds
Queue::subscribeWithLimit(callable $callback, int $maxMessages, int $waitTimeout)— structurally identical to the existingQueue::subscribe(), with the addition of abasic_cancelcall once$maxMessageshave been processed.CallbackInvoker::invoke()now routes AMQP queues with a message limit through this method instead of thedequeue()loop.As part of this change, the duplicated AMQP message-to-envelope property mapping that existed separately in
subscribe()and the new method has been extracted into a private helpercreateEnvelopeFromAmqpMessage().The existing
dequeue()polling loop is preserved unchanged for all other queue backends (MySQL, STOMP).Behaviour matrix:
--max-messages(cron)basic_getloop — 1 RTT/messagebasic_consume+ prefetch — batch pushbasic_consumeviasubscribe()basic_consumeviasubscribe()(unchanged)--max-messagesdequeue()loopdequeue()loop (unchanged)The
consumers_wait_for_messagesdeployment config is respected:1(default):channel->wait()blocks indefinitely — same as original0: 1-second idle timeout onchannel->wait(), exits promptly when the queue drains — matches original immediate-exit behaviourThe poison pill check runs before each message callback, consistent with the original implementation.
Benchmark
Environment: Warden / Docker on Apple M-series, RabbitMQ co-located in the same Docker network (~0.1 ms RTT).
Method: A self-contained PHP script bootstraps Magento to obtain the AMQP connection config, declares a temporary durable queue, publishes N messages, then times each approach independently before deleting the queue. Both rounds use identical message payloads and per-message
basic_ack.Benchmark script (click to expand)
Results — 5,000 messages, prefetch=100 (3 runs):
basic_getbeforebasic_consumeafterPrefetch value comparison (5,000 messages, single run each):
These numbers are from a co-located Docker environment where RTT is ~0.1 ms. In a production deployment with a dedicated RabbitMQ host the difference is proportionally larger — at a typical 2 ms RTT:
basic_get: 5,000 × 2 ms = ~10 seconds of pure queue overhead per cron consumer runbasic_consume: ~50 round-trips × 2 ms = ~0.1 secondsManual testing scenarios
Reproduce the benchmark
Real consumer end-to-end — trigger a mass product attribute update from Admin to populate the
product_action_attribute.updatequeue, then:Verify products are updated and
var/log/is clean.MySQL queue unaffected — configure a consumer on a
dbconnection and confirm the originaldequeue()loop is still used (nosubscribeWithLimitcall).consumers_wait_for_messages=0— set inenv.php, run a consumer against an empty queue, confirm it exits within ~1 second instead of blocking indefinitely.--max-messages=0edge case — confirm the consumer exits immediately without connecting to RabbitMQ (guarded byif ($maxMessages <= 0) { return; }).Related Pull Requests
Fixed Issues (if relevant)
Questions or comments
This PR is intentionally narrow in scope: only
CallbackInvokerandQueueare changed, and only for the AMQP + limited-message path. Thedequeue()method is left untouched. Happy to split into smaller commits or add integration test coverage if the maintainers prefer.Contribution checklist