sepuri sai krishna created CASSANDRA-21575:
----------------------------------------------
Summary: Message ids become negative once the id counter wraps,
inflating every message header
Key: CASSANDRA-21575
URL: https://issues.apache.org/jira/browse/CASSANDRA-21575
Project: Apache Cassandra
Issue Type: Bug
Components: Messaging/Internode
Reporter: sepuri sai krishna
Assignee: sepuri sai krishna
{{Message.nextId()}} draws from a shared {{AtomicInteger}} and widens the
result to a {{long}}:
{code:java}
private static final AtomicInteger nextId = new AtomicInteger(0);
private static long nextId()
{
long id;
do
{
id = nextId.incrementAndGet();
}
while (id == NO_ID);
return id;
}
{code}
Once the counter passes {{Integer.MAX_VALUE}} it wraps to
{{Integer.MIN_VALUE}}, and the negative
{{int}} sign-extends into a negative {{long}}. {{Message.Serializer}} writes
the id as an unsigned
vint:
{code:java}
out.writeUnsignedVInt(header.id);
{code}
A negative long has all its high bits set, so it always encodes at the maximum
width of 9 bytes:
|| id || wire bytes ||
| 1 | 1 |
| 2147483647 | 5 |
| -2147483648 (first wrap) | 9 |
| -1 | 9 |
That is an extra 4 bytes on the header of every internode message, sustained
for the 2^31 messages
until the counter cycles back through the positive range, and then again on
every subsequent wrap.
A node sending 50k messages/sec reaches the first wrap in roughly 12 hours, so
this is steady state
for any long-running cluster rather than an edge case.
Nothing depends on the sign of an id. They are opaque correlation keys, used
only for matching
responses to callbacks in {{RequestCallbacks}}, so keeping them non-negative is
behaviour preserving.
The wire format is unchanged; only the range of values written changes.
h3. Coupled defect in ForwardingInfo
{{ForwardingInfo}} is the {{FORWARD_TO}} parameter used for inter-DC write
forwarding. It writes ids
with the 64-bit form and sizes them the same way, but reads them back with the
32-bit form:
{code:java}
// serialize
out.writeUnsignedVInt(ids[i]);
// serializedSize
size += computeUnsignedVIntSize(ids[i]);
// deserialize
ids[i] = in.readUnsignedVInt32(); // mismatch
{code}
This is the only place in the codebase that reads a message id at 32 bits;
every other reader uses
{{readUnsignedVInt}}/{{getUnsignedVInt}}. It is currently harmless only by
accident: ids always fit in
{{int}} today, because they come from an {{int}} counter, so the
{{checkedCast}} inside
{{readUnsignedVInt32}} happens to succeed even for the sign-extended negative
values.
The same narrowing is present on 4.0 and 4.1, spelled differently:
{code:java}
ids[i] = version >= VERSION_40 ? Ints.checkedCast(in.readUnsignedVInt()) :
in.readInt();
{code}
so both parts of this ticket apply from 4.0 onwards. The attached patch is
against trunk; happy to
prepare backports for whichever branches the reviewer wants this on.
That makes it a blocker for the fix rather than a separate cleanup. Once ids
span the full unsigned
32-bit range, this read throws:
{noformat}
fixed nextId, post-wrap id=2147483648 ForwardingInfo.deserialize:
VIntOutOfRangeException
fixed nextId, max id=4294967295 ForwardingInfo.deserialize:
VIntOutOfRangeException
{noformat}
which would break inter-DC write forwarding. Both changes therefore belong in
the same commit.
h3. Fix
Mask the counter so ids stay in {{[0, 2^32)}}. This covers the same number of
distinct values, keeps
them non-negative, and bounds the encoding at 5 bytes. Change
{{ForwardingInfo.deserialize}} to use
{{readUnsignedVInt}}, matching its own serialize and serializedSize.
h3. Tests
The existing {{ForwardingInfoTest}} only ever used ids 44-49, which is why the
width mismatch was
never exercised. Added:
* {{ForwardingInfoTest.testLargeMessageIdsRoundTrip}} - round-trips ids across
the whole unsigned
32-bit range, including {{Integer.MAX_VALUE + 1}} and {{0xFFFFFFFF}}, asserting
{{serializedSize}}
matches the bytes written. Fails with {{VIntOutOfRangeException: 2147483648 is
out of range for a
32-bit integer}} without the {{ForwardingInfo}} fix.
* {{MessageTest.testIdsRemainUnsignedAcrossCounterWrap}} - asserts ids stay
non-negative, stay within
32 unsigned bits, and encode within 5 bytes across the wrap boundary.
* {{MessageTest.testLargeIdRoundTrips}} - round-trips a message header carrying
ids from the upper
half of the range, for every supported messaging version.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
