Conversation
Adds two opt-in producer options, `confirm_select` and `confirm_timeout`,
which put the producer's channel into AMQP publisher-confirm mode and make
`publish()` wait for the broker's confirmation.
producers:
upload_picture:
connection: default
exchange_options: {name: 'upload-picture', type: direct}
confirm_select: true
confirm_timeout: 10.0
Today `publish()` cannot distinguish "written to the socket" from "accepted
by the broker", so applications needing at-least-once semantics have to
reach past the bundle onto AMQPChannel — and then lose confirm mode
silently whenever the bundle recreates the channel.
Implementation notes:
* Confirm mode is enabled in an overridden getChannel(), keyed on channel
object identity, so it survives reconnects, channel recreation after a
close, and explicit setChannel() calls.
* The extension adds setConfirmSelect()/setConfirmationTimeout() only when
the feature is switched on, so no existing producer service definition
changes shape. confirm_timeout is cast to float at the wiring site,
since Symfony's FloatNode accepts an int without casting it.
* publish() now returns bool. It previously returned nothing, and returns
true whenever confirms are disabled, so no caller can start seeing a
falsy value it did not see before.
* A confirmation timeout propagates as AMQPTimeoutException rather than
being folded into `return false`, so a broken broker cannot be mistaken
for a rejected message.
Adds Tests/RabbitMq/ProducerTest.php (there was no producer test) plus two
extension tests — one asserting the wiring when the option is set, one
asserting that producers without it get no extra method calls.
README and CHANGELOG updated.
Author
|
The failing Scrutinizer check here is unrelated to this change — its build errors out while installing dependencies, before any analysis runs: All 83 packages fail the same way, and with source fallback disabled the build exits 100. That looks like the Scrutinizer integration's GitHub credentials having expired rather than anything in the diff. It does not seem specific to this PR: #747 shows the same Separately, the GitHub Actions runs are sitting at "action_required" since this is my first contribution here — if someone gets a chance to approve them, the PR would get real CI results. The same commit is green on my fork's CI. |
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
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.
What
Adds two opt-in producer options,
confirm_selectandconfirm_timeout, which put the producer's channel into AMQP publisher-confirm mode and makepublish()wait for the broker's confirmation.Why
Today
publish()cannot distinguish "written to the socket" from "accepted by the broker". A broker that is out of disk, is refusing publishes, or dies mid-publish is indistinguishable from success.Applications that need an at-least-once guarantee therefore have to reach past the bundle onto
AMQPChanneldirectly — which means re-implementing the ack/nack handler wiring on every reconnect, and losing confirm mode silently whenever the bundle recreates the channel underneath them.Prior art
This was proposed once before, in #487 (2017), addressing #460 and #74. It attracted repeated interest from users over three and a half years, received no maintainer review, and was auto-closed by the stale bot in April 2021 — no objection to the feature itself was ever raised.
This is a fresh implementation against current
master, not a rebase of that branch.Design notes
Confirm mode is enabled in an overridden
getChannel(), keyed on channel object identity.Confirm mode is a property of the channel, not of the producer, and the bundle replaces the channel in three situations:
BaseAmqp::getChannel()recreates it once closed,BaseAmqp::setChannel()swaps it, andBaseAmqp::reconnect()reconnects underneath it. Enabling it once at construction time would silently drop the producer back to fire-and-forget on any of those, whilepublish()kept reporting success. Comparing the channel object (not the channel id, which is reused after a reconnect) covers all three cases in one branch.The extension adds
setConfirmSelect()/setConfirmationTimeout()only when the feature is switched on, so no existing producer service definition changes shape — the existinggetMethodCalls()assertions inOldSoundRabbitMqExtensionTestneeded no edits. There is a regression test pinning that.publish()now returnsbool. It previously returned nothing, and it returnstruewhenever confirms are disabled, so no caller can start seeing a falsy value it did not see before.$acknowledgedis reset before everybasic_publish(), so one nacked message cannot poison the result of the next publish.A confirmation timeout propagates as
AMQPTimeoutExceptionrather than being folded intoreturn false. A broker that stops confirming is an infrastructure failure, not a per-message rejection, and collapsing the two would hide it behind the same value a legitimate nack produces.confirm_timeout: 0maps to php-amqplib's "wait indefinitely".Note that
isConfirmSelect()is added toProducerand deliberately not toProducerInterface— adding a method to a published interface would be a fatal error for third-party implementations. Callers typed against the interface need aninstanceofcheck. Happy to raise that as a separate issue for the next major if you think the interface should carry it.Backwards compatibility
None affected. The feature is opt-in and defaults to off. With it off, the only observable change is
publish()returningtrueinstead ofnull, and the channel is not touched at all — there is a test assertingconfirm_select()andwait_for_pending_acks()are never called in that case.Tests
Adds
Tests/RabbitMq/ProducerTest.php— there was no producer test before. Seven cases:publish()returnstrueconfirm_timeoutdefaults to10.0publish()returnsfalseonbasic.nackAMQPTimeoutExceptionPlus two extension tests: one asserting the wiring when the option is set, one asserting that producers without it get no extra method calls.
Green on the full matrix (PHP 8.2/8.3/8.4 × Symfony 6.4/7.4/8.0), php-cs-fixer clean.
Docs
README gains a "Publisher Confirms" subsection under "Producer" and two lines in the main configuration sample; CHANGELOG updated — as per the contribution guidelines.