[jira] [Commented] (QPID-8751) [Broker-J] AMQP 1.0 consumer is never resumed when the session incoming window is reopened by a flow naming another link

["Robert Godfrey (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/QPID-8751?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18104157#comment-18104157 ] 

Robert Godfrey commented on QPID-8751:
--------------------------------------

Thanks for this Marco - I'll try to take a look at this before the end of the week

> [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
>            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: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org