Skip to content

Preserve the deadline in IceGrid ServerEntry synchronization waits #6641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bernardnormier merged 1 commit into from
Aug 18, 2026
Merged

Preserve the deadline in IceGrid ServerEntry synchronization waits #6641

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 6 additions & 15 deletions cpp/src/IceGrid/ServerCache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,22 +707,13 @@ void
ServerEntry::waitImpl(chrono::seconds timeout)
{
unique_lock lock(_mutex);
if (timeout != 0s)
if (timeout > 0s)
{
while (_synchronizing)
{
if (timeout > 0s)
{
if (_condVar.wait_for(lock, timeout) == cv_status::timeout)
{
break; // Timeout
}
}
else
{
_condVar.wait(lock);
}
}
_condVar.wait_until(lock, chrono::steady_clock::now() + timeout, [this] { return !_synchronizing; });

@bernardnormier bernardnormier Aug 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The symptom is a late SynchronizationException, and only under sustained allocation churn on the same entry — notifications must keep arriving faster than the timeout to extend the wait. Nothing user-visible changes during normal operations, so no changelog entry.

Comment on lines +710 to +712

@bernardnormier bernardnormier Aug 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Asserting that the exception lands at the original deadline means a wall-clock timing assertion while generating cross-thread notification churn — that's a flaky test, not a regression guard. The change is the standard deadline-with-predicate idiom; the existing allocation suite covers the functional behavior.

}
else if (timeout < 0s)
{
_condVar.wait(lock, [this] { return !_synchronizing; });
}
if (_synchronizing) // If we are still synchronizing, throw SynchronizationException
Comment on lines 717 to 718

@externl externl Aug 18, 2026

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.

Reaching this if (_synchronizing) is impossible. Can we just replace with an assert?

@InsertCreativityHere InsertCreativityHere Aug 18, 2026

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.

Couldn't we still hit this when timeout > 0s, and we hit the timeout?
wait_until doesn't throw I don't think. It just returns false (if we hit the timeout).

@externl externl Aug 18, 2026

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.

Yes, you're right I didn't read this very well.

{
Expand Down
Loading