[
https://issues.apache.org/jira/browse/QPID-8751?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Marco Geri updated QPID-8751:
-----------------------------
Description:
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}}
was:
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
sendability
transitions 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}}
> [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: [email protected]
For additional commands, e-mail: [email protected]
