[
https://issues.apache.org/jira/browse/QPID-8751?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Robert Godfrey reassigned QPID-8751:
------------------------------------
Assignee: Robert Godfrey
> [Broker-J] AMQP 1.0 consumer is never resumed when the session incoming
> window is reopened by a flow naming another link
> ------------------------------------------------------------------------------------------------------------------------
>
> Key: QPID-8751
> URL: https://issues.apache.org/jira/browse/QPID-8751
> Project: Qpid
> Issue Type: Bug
> Components: Broker-J
> Affects Versions: qpid-java-broker-10.1.0
> Environment: Reproduced against Broker-J 10.1.0. The affected code is
> unchanged on {{main}} and on the
> 6.1.x, 7.1.x and 8.0.x branches.
> Reporter: Marco Geri
> Assignee: Robert Godfrey
> Priority: Critical
> Attachments: QPID-XXXX-proposed-fix.patch,
> QPID-XXXX-reproducer-test.patch
>
>
> h2. Problem
> On an AMQP 1.0 session with two or more sending links, a consumer suspended
> because the session incoming window was exhausted is never resumed. The link
> keeps its credit and stays attached, but the queue never serves it again for
> the life of the connection, and SUB-1003 is logged for it without bound. Only
> reconnecting clears it.
> h2. Reproduce
> One session, two consumers A and B on the same queue:
> || ||Event||A||B||
> |1|both links are granted link credit|sendable|sendable|
> |2|the broker fills the session incoming window sending transfers, and each
> consumer is suspended the next time the queue tries to deliver to it, because
> {{ConsumerTarget_1_0#allocateCredit}} refuses|suspended|suspended|
> |3|the peer settles a delivery on B and replenishes the window with {{{}flow
> {handle=B{}}}}|*still suspended*|sendable|
> After step 3 the session can send again and A still holds link credit on an
> attached link, but nothing ever re-evaluates A.
> Two or more links on the same session are required: with a single link the
> replenishing flow necessarily names it, so it recovers.
> h2. Cause
> {{Session_1_0#receiveFlow}} recomputes the session scoped
> {{{}_remoteIncomingWindow }}unconditionally, but notifies at most one link -
> the one named by the flow's optional \{{{}handle{}}}:
> {code:java}
> _remoteIncomingWindow = ...; // session scoped
> ...
> if (handle != null) { endpoint.receiveFlow(flow); } // only this link is
> re-evaluated
> else { /* all links */ }
> {code}
> That value feeds *every* sending link's sendability, through
> {{SendingLinkEndpoint#hasCreditToSend()}} and
> {{{}ConsumerTarget_1_0#updateNotifyWorkDesired(){}}}.
> A session scoped change is therefore delivered as a link scoped notification,
> and a link not named by the flow is never told the session can send again.
> There is no second chance: a suspended consumer is offered no message, so its
> own link credit never changes, and a well behaved peer has no reason to send
> a flow naming its handle.
> h2. Why the handle-less branch does not save us
> The {{handle}} field is optional (2.7.4), but both Qpid Java clients always
> set it on the frame that replenishes the session window, so that branch is
> effectively dead:
> * protonj2: {{ProtonSessionIncomingWindow#deliveryRead}} ->
> {{ProtonSession#writeFlow(link) }}-> {{ProtonReceiver#decorateOutgoingFlow}}
> -> {{Flow#setHandle}}
> * proton-j: {{TransportImpl#processSenderFlow}} writes a per receiver flow
> carrying that receiver's handle whenever the incoming window is zero
> The first link to replenish the window is resumed; the others are stranded.
> h2. Attachments
> *{{QPID-XXXX-reproducer-test.patch}}* - a unit test for
> {{{}Session_1_0Test{}}}. Applied on its own to {{main}} it fails with:
> {noformat}
> org.opentest4j.AssertionFailedError: Consumer A should be resumed once the
> session
> window is reopened by a flow naming another link ==> expected: <true> but
> was: <false>
> {noformat}
> It closes the window with an explicit flow carrying {{incoming-window=0}}
> rather than by sending transfers. Both paths end with
> {{{}_remoteIncomingWindow == 0{}}}, and the flow form keeps the test free of
> message delivery.
> *{{QPID-XXXX-proposed-fix.patch}}* - a possible fix, offered as a starting
> point rather than a finished change. It sweeps the remaining link endpoints
> when session sendabilitytransitions from false to true, mirroring what
> {{transportStateChanged}} already does for the connection scoped condition.
> Only the resume direction needs the sweep, since losing session credit is
> already handled accurately by {{allocateCredit}} refusing the next delivery
> attempt. With both patches applied, {{Session_1_0Test}} passes on
> {{{}main{}}}.
> This one would benefit from review by a maintainer familiar with the link and
> session flow control code. Two points I am unsure about:
> * whether the sweep should be restricted to {{{}SendingLinkEndpoint{}}}, as
> {{transportStateChanged}} does -
> {{AbstractReceivingLinkEndpoint#flowStateChanged}} is a no-op and
> {{ErrantLinkEndpoint#flowStateChanged}} throws
> * whether the transition should be evaluated before
> {{endpoint.receiveFlow(flow)}} rather than after, since that call can consume
> the freshly granted window via {{_resumeAcceptedTransfers}}
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
