Do not warn on benign double completion of backup-aware invocations [HZ-5459]#1475
Merged
ihsandemir merged 3 commits intoJun 4, 2026
Conversation
ihsandemir
changed the title
dmitriyrazboev
previously approved these changes
Jun 4, 2026
JackPGreen
reviewed
Jun 4, 2026
| class address; | ||
|
|
||
| namespace test { | ||
| // White-box test fixture; granted access to private completion internals to |
Contributor
There was a problem hiding this comment.
Can we use "exposed for testing" or some other terminology in use elsewhere in the codebase?
JackPGreen
reviewed
Jun 4, 2026
| @@ -0,0 +1,185 @@ | |||
| /* | |||
| * Copyright (c) 2008-2025, Hazelcast, Inc. All Rights Reserved. | |||
Contributor
There was a problem hiding this comment.
Suggested change
| * Copyright (c) 2008-2025, Hazelcast, Inc. All Rights Reserved. | |
| * Copyright (c) 2008-2026, Hazelcast, Inc. All Rights Reserved. |
Not sure how were managing this.
Contributor
Author
There was a problem hiding this comment.
moved this file content into existing HazelcastTests5.cpp file.
ihsandemir
force-pushed
the
fix/spurious-failed-to-set-response-warning
branch
2 times, most recently
from
June 4, 2026 08:34
7bf3fe5 to
71c9163
Compare
For a backup-aware operation the primary response (notify_response) and the last backup ack (notify_backup) can race to complete the invocation. Both paths complete with the same pending response, so the loser of the race hits boost::promise_already_satisfied. The future is still completed exactly once with the correct value and the duplicate is harmlessly dropped, but complete() logged every occurrence at WARNING, flooding the logs of healthy clients under load. The race became routinely observable after the multi-threaded pipeline work, which made the response and the backup ack run on different threads. Mirror the Java client's AbstractInvocationFuture.warnIfSuspiciousDouble- Completion: only warn when the offered response differs from the canonical pending_response_. A duplicate completion with the same response is logged at finest, while a genuinely conflicting value still warns. This also matches how set_exception() already treats promise_already_satisfied. Add regression tests that drive the private completion path via friend access to assert the benign duplicate is not a warning and a conflicting one still is. Fixes hazelcast#1474
ihsandemir
force-pushed
the
fix/spurious-failed-to-set-response-warning
branch
from
June 4, 2026 08:34
71c9163 to
56204de
Compare
JackPGreen
approved these changes
Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1474
Problem
Since 5.7.0 a healthy C++ client connected to a multi-member cluster continuously emits
WARNINGlines like:A 10-client soak run produced 47,664 of these, all for backup-aware operations with
backup_acks_expected == backup_acks_received, with no actual failures.Root cause
For a backup-aware operation the invocation completes once the primary response and the backup ack(s) have arrived. Both
notify_response()andnotify_backup()can win the final completion, and there is an inherent race in which both reachcomplete()with the same pending response:notify_responsestagespending_response_notify_backup(other thread) increments the ack count and completes the promisenotify_response's recheck now seesreceived == expected, falls through and callscomplete()again →boost::promise_already_satisfiedThe future is completed exactly once with the correct value; the duplicate is harmlessly dropped. Only the log line is wrong. The race became routine after the multi-threaded pipeline work (#1412), which made the response and the backup ack run on different threads.
This mirrors the Java client (
BaseInvocation.notifyResponsehas the same double-check). The difference is reporting: Java'sAbstractInvocationFuture.warnIfSuspiciousDoubleCompletiononly warns when the already-set value differs from the offered one, so the same-value backup-ack race is silent. The C++complete()warned on everypromise_already_satisfied.Fix
complete()now mirrors Java: a duplicate completion with the same response (the benign backup-ack race) is logged atfinest, while a genuinely conflicting value still warns. This also aligns the value path with howset_exception()already handles the same condition.Tests
Added
client_invocation_testwith two cases driving the private completion path via friend access:WARNING, onefinestWARNINGVerified red/green: the benign test fails before the fix (logs at
WARNING) and passes after. Both tests pass against a running cluster.