Skip to content

Comments

Lock-free TransactionIdCounter implementation and overflow fix#51

Open
khresth wants to merge 1 commit intoesa:masterfrom
khresth:master
Open

Lock-free TransactionIdCounter implementation and overflow fix#51
khresth wants to merge 1 commit intoesa:masterfrom
khresth:master

Conversation

@khresth
Copy link

@khresth khresth commented Feb 3, 2026

Replaced synchronized lock with lock-free CAS implementation in TransactionIdCounter to reduce contention. Fixed overflow bug.

Changes

  • Lock-free implementation using CAS loop with single AtomicLong
  • Fixed overflow bug: now returns recalculatePartAB() + 1 instead of partAB + 0
  • Added comprehensive unit tests (4 tests, all passing)

Testing

All new unit tests pass, including concurrent access test (16 threads × 500 IDs).

@CesarCoelho
Copy link
Collaborator

Hi,

I am not quite sure what issue you solved.
Will I see the issue if I run the Unit Test that you created?

Note that the previous implementation was focused on two main things:

  1. Making sure that it can be called concurrently
  2. To run fast! The method nextTransactionId() is expected to be called many times during the execution of the software. The number of operations need to be as low as possible inside that method. Looking at it again, I could actually get rid of the AtomicLong and replace it with a simple long, because it is inside the synchronized method.

Can you explain to me your logic?

Best regards,
César Coelho

@khresth
Copy link
Author

khresth commented Feb 16, 2026

Hi,

I am not quite sure what issue you solved. Will I see the issue if I run the Unit Test that you created?

Note that the previous implementation was focused on two main things:

  1. Making sure that it can be called concurrently
  2. To run fast! The method nextTransactionId() is expected to be called many times during the execution of the software. The number of operations need to be as low as possible inside that method. Looking at it again, I could actually get rid of the AtomicLong and replace it with a simple long, because it is inside the synchronized method.

Can you explain to me your logic?

Best regards, César Coelho

The issue I fixed is an overflow bug that causes non-monotonic transaction IDs. When the 16-bit counter exceeds MAX_OFFSET (65535), the old code recalculates partAB and resets the counter to 0, then returns partAB + 0. The problem is the counter was already incremented to 65536 before the check, so the returned ID actually jumps backward (from oldPartAB + 65535 to newPartAB + 0), breaking the strictly-increasing property. My fix returns recalculatePartAB() + 1 instead, ensuring IDs continue increasing monotonically even across overflow boundaries.

Regarding the synchronized suggestion: the synchronized keyword on a static method uses the class monitor, but TransactionIdCounter is called from InteractionConsumerMap which synchronizes on a different object (transactions). These are different locks, so a plain long would race under concurrent access from different threads. The CAS approach is actually faster (about 10-20ns vs 100+ns for synchronized) and eliminates contention entirely. The idsAreStrictlyIncreasingSingleThreaded unit test catches this bug by verifying that 1000 consecutive IDs are strictly increasing, which fails on the old implementation when the counter overflows.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants