Skip to content

Replace std::default_random_engine with std::mt19937 (humble) #2847

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.

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

Open
wants to merge 1 commit into
base: humble
Choose a base branch
from

Conversation

keeponoiro
Copy link

This PR addresses issue #2842, where std::default_random_engine can lead to periodic goal ID duplication due to its underlying implementation in GCC (minstd_rand0). When actions are executed frequently, this periodicity increases the likelihood of duplicate goal IDs, potentially causing unintended behavior.

In extreme cases, duplicate goal IDs can:

Trigger an exception when attempting to accept a new goal (goal ID already exists).

Extend the expiration time of the first goal when a duplicate ID is encountered.

To mitigate this, this PR replaces std::default_random_engine with std::mt19937, which provides a higher-quality random number generator with better distribution properties, reducing the risk of goal ID collisions.

Changes in this PR
std::default_random_engine → std::mt19937 in client_base.cpp

Ensures goal ID randomness to prevent collisions in high-frequency action executions.

Testing
In a 13-hour test, we observed periodic goal ID duplication when using std::default_random_engine.

Same goal ID appeared twice within a 10-minute period.

When goal ID retention was reduced to 1 minute, the issue no longer occurred.

With std::mt19937, goal ID duplication is effectively mitigated.

Impact
No ABI breakage.

Improves reliability of goal ID generation.

Addresses unintended goal expiration extension issues.

Signed-off-by: keeponoiro <keeeeeeep@gmail.com>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm with green CI.

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