Marco Geri created QPID-8751:
--------------------------------
Summary: [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
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
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}}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
