aeron icon indicating copy to clipboard operation
aeron copied to clipboard

Erroneous next fragment term offset computation

Open jrsala-auguration opened this issue 2 years ago • 0 comments

This is mostly a nitpick, and I'm not sure my analysis below is correct, so please correct me if I'm wrong.

In FragmentAssembler#handleFragment, the term offset of the next fragment is computed as

BitUtil.align(offset + length + HEADER_LENGTH, FRAME_ALIGNMENT)

offset and length are (most likely) provided by TermReader#read so their values are frameOffset + HEADER_LENGTH and frameLength - HEADER_LENGTH respectively, and therefore offset + length is equal to frameOffset + frameLength. Aligning that value to the FRAME_ALIGNMENT to obtain the offset of the next frame makes sense. But adding + HEADER_LENGTH before doing the alignment doesn't.

It just so happens that HEADER_LENGTH is equal to FRAME_ALIGNMENT and so the operations x -> BitUtil.align(x, FRAME_ALIGNMENT) and x -> x + HEADER_LENGTH commute. But if HEADER_LENGTH was not a multiple of FRAME_ALIGNMENT then this code would break. So for cleanliness and to avoid a bad surprise if the transport protocol ever changes (unlikely I guess), it would be best to instead write BitUtil.align(offset + length, FRAME_ALIGNMENT) + HEADER_LENGTH.

jrsala-auguration avatar Sep 29 '22 12:09 jrsala-auguration