Skip to content

[#2404] WIP: Network advisory clean-ups - #2405

Draft
mattrpav wants to merge 3 commits into
apache:mainfrom
mattrpav:amq-gh-2404-network-advisory-dispatch
Draft

[#2404] WIP: Network advisory clean-ups#2405
mattrpav wants to merge 3 commits into
apache:mainfrom
mattrpav:amq-gh-2404-network-advisory-dispatch

Conversation

@mattrpav

@mattrpav mattrpav commented Aug 5, 2026

Copy link
Copy Markdown
Contributor
  • dispatch counter race fix
  • timed task to ack advisory messages instead of relying on count-based ack percentage

This hardens network consumers from appearing slow or hung due to outstanding ack's and being unintentionally closed due to slow consumer strategy.

demandConsumerDispatched is a plain int incremented and reset from
multiple transport/executor threads in ackAdvisory. The unsynchronized
read-modify-write and unguarded reset let concurrent threads claim
overlapping counter ranges in their advisory acks (observed: 800k
dispatches acked as 1.16M) or lose counted dispatches outright,
corrupting the remote advisory prefetch window. Widens ackAdvisory to
package-private for the test.
Replace the plain-int demandConsumerDispatched with an AtomicInteger.
The threshold check now claims the observed count via compareAndSet
before acking, so concurrent advisory deliveries can neither lose
increments nor ack overlapping counter ranges; a losing thread's
increment stays counted for a later advisory to claim (ack lag bounded
by one message).
@mattrpav mattrpav self-assigned this Aug 5, 2026
@mattrpav mattrpav changed the title [#2404] Network advisory dispatch race fix [#2404] Network advisory dispatch counter race fix Aug 5, 2026
@mattrpav
mattrpav marked this pull request as draft August 5, 2026 20:27
@mattrpav mattrpav changed the title [#2404] Network advisory dispatch counter race fix [#2404] WIP: Network advisory dispatch counter race fix Aug 5, 2026
… not aborted as slow

The bridge's demand advisory consumer batched acks purely by
advisoryAckPercentage, so a bridge holding a partial batch below the
threshold never acked during quiet periods.

New bridge config advisoryAckInterval (default 15000 ms, <= 0 disables)
acknowledges pending advisory dispatches once the interval elapses: the
@mattrpav mattrpav changed the title [#2404] WIP: Network advisory dispatch counter race fix [#2404] WIP: Network advisory clean-ups Aug 5, 2026
@cshannon cshannon linked an issue Aug 6, 2026 that may be closed by this pull request
@cshannon

cshannon commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Responding to: #2404 (comment)

Flushing outstanding acks probably makes sense anyways, but the motivation seems weird to me. (trying to work around stale connections) Anytime you do batching it usually makes sense to have a time limit to flush so you don't wait forever. In fact, I've had to configure bridges to lower the percentage for acking advisories for bridges that are not as busy just so we don't show pending acks for a long time in metrics.

I am mostly just curious why you need to use this as a work around for bad connections? Does the inactivity monitor not work for your use case? That's the tool to detect bad connections and to cause resetting if there are no heart beats, so it seems strange to rely on this for that reason. I would think if the connection is bad the broker would stop getting heart beats.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

Network Connector dispatch usage of integer creates race

2 participants