Skip to content
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

Fix sanitizer issues [BMQ,MWC] #373

Merged
merged 36 commits into from
Aug 29, 2024

Conversation

alexander-e1off
Copy link
Collaborator

Revise cancelled/suppressed unit tests running under sanitizers. Fix some issues.

alexander-e1off and others added 21 commits March 28, 2024 18:28
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
@alexander-e1off alexander-e1off requested a review from a team as a code owner July 24, 2024 13:30
reqOptions.setAttemptInterval(
bsls::TimeInterval(0, 10 * bdlt::TimeUnitRatio::k_NS_PER_MS));

reqOptions.setAttemptInterval(bsls::TimeInterval(0, 1));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I understand why the second call of setAttemptInterval with such strange arguments (1 ns) is required.

@678098 678098 self-assigned this Jul 24, 2024
@678098 678098 self-requested a review July 24, 2024 14:03
etc/ubsansup.txt Outdated
# option is to update the code, which can be done by someone feeling
# adventurous.
alignment:crc32c1024SseInt
# UndefinedBehaviorSanitizer suppressions file for BMQ.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# UndefinedBehaviorSanitizer suppressions file for BMQ.
# UndefinedBehaviorSanitizer suppressions file for BlazingMQ.

Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
alexander-e1off and others added 8 commits July 25, 2024 17:37
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
Signed-off-by: Aleksandr Ivanov <aivanov71@bloomberg.net>
@678098 678098 self-requested a review August 14, 2024 14:14
etc/tsansup.txt Outdated
# point.
race:TestSession::waitForChannelClose
race:TestSession::arriveAtStepWithCfgs
# Suppress sporadically appearing data race in bmqimp::Brokersession test driver.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# Suppress sporadically appearing data race in bmqimp::Brokersession test driver.
# Suppress sporadically appearing data race in bmqimp::BrokerSession test driver.

etc/tsansup.txt Outdated
# at nearly same time in other thread bmqimp::BrokerSession::onConfigureQueueResponse() calls
# queue->options().suspendsOnBadHostHealth() method which is detected as data race.
# bmqt::QueueOptions and bmqimp::Queue classes are not thread safe by design,
# and bmqimp::BrokerSession::onConfigureQueueResponse() cllback access them in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# and bmqimp::BrokerSession::onConfigureQueueResponse() cllback access them in
# and bmqimp::BrokerSession::onConfigureQueueResponse() callback access them in

@@ -248,6 +248,10 @@ class TestChannel : public Channel {
/// Pops a close-call from those written to the channel (FIFO ordering).
CloseCall popCloseCall();

/// Lock mutex and return `true` if d_closeCalls collection is empty,
/// `false` otherwise.
bool closeCallsEmpty();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might qualify as const and make mutex mutable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed as proposed.

Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

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

Build 185 of commit c674af6 has completed with FAILURE

Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

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

Build 205 of commit 3ff4329 has completed with FAILURE

Copy link
Collaborator

@678098 678098 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

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

Build 211 of commit db49b5d has completed with FAILURE

@alexander-e1off alexander-e1off merged commit 4dd944b into bloomberg:main Aug 29, 2024
24 checks passed
syuzvinsky pushed a commit to syuzvinsky/blazingmq that referenced this pull request Aug 30, 2024
alexander-e1off added a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
alexander-e1off added a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
alexander-e1off added a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
@alexander-e1off alexander-e1off deleted the fix-sanitizer-issues branch October 31, 2024 09:20
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