Skip to content

[Java] Remove Batch.EMPTY Shared Objects#2495

Merged
batiati merged 3 commits into
mainfrom
batiati-java_fix_batch_header
Nov 21, 2024
Merged

[Java] Remove Batch.EMPTY Shared Objects#2495
batiati merged 3 commits into
mainfrom
batiati-java_fix_batch_header

Conversation

@batiati

@batiati batiati commented Nov 21, 2024

Copy link
Copy Markdown
Contributor

Since the introduction of batch.getHeader() by #2481, we must create a new instance per request instead of sharing the same Batch.EMPTY instance.
However, to reduce allocations, the underlying buffer is still a shared empty object.

This PR fixes a problem where concurrent requests with reply.len == 0 were overriding each other's headers.

Since the introduction of `batch.getHeader()` by #2481, we must create a new instance per request. However, to reduce allocations, the underlying buffer is a shared empty object.

This PR fixes a problem where concurrent requests with `reply.len == 0` were overriding each other's headers.
sentientwaffle
sentientwaffle previously approved these changes Nov 21, 2024

@sentientwaffle sentientwaffle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- thanks for tracking this down!

The original bug can be reproduced with the integration test:

    @Test
    public void testRace() throws Throwable {
        try (final var client2 = new Client(clusterId, new String[] {server.getAddress()})) {
            for (int i = 0; i < 1000; i++) {
                var r1 = client.lookupAccountsAsync(new IdBatch(UInt128.id()));
                var r2 = client2.lookupAccountsAsync(new IdBatch(UInt128.id()));

                var t1 = r1.get().getHeader().getTimestamp();
                var t2 = r2.get().getHeader().getTimestamp();
                if (t1 == t2) {
                    throw new Exception("BUG: " + String.format("timestamp: %d = %d", t1, t2));
                }
            }
        }
    }

...Though that is niche enough that it probably isn't worth adding to the integration test suite.

*/
@Test
public void testEmptyReply() throws Throwable {
final int TASKS_COUNT = 1_000;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to add this to the integration test suite, let's change the task count to 4 or so... it didn't take that many repetitions to hit the bug, and I don't want to make the test suite slower 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, it's about 300ms now!

@batiati
batiati enabled auto-merge November 21, 2024 15:58
@batiati
batiati added this pull request to the merge queue Nov 21, 2024
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 21, 2024
@batiati
batiati added this pull request to the merge queue Nov 21, 2024
Merged via the queue into main with commit 584610e Nov 21, 2024
@batiati
batiati deleted the batiati-java_fix_batch_header branch November 21, 2024 17:44
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