Skip to content

Refactor[bmqsys]: Remove bmqsys_threadutil, use bslmt_threadutil instead #989

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactor[bmqsys]: Remove bmqsys_threadutil, use bslmt_threadutil instead #989

wants to merge 1 commit into from

Conversation

MorganaFuture
Copy link

@MorganaFuture MorganaFuture commented Oct 18, 2025 •
edited
Loading

Fixes Ticket 950
Remove bmqsys::ThreadUtil wrapper which duplicated bslmt::ThreadUtil functionality from BDE. Migrate all usages to bslmt::ThreadUtil and remove redundant files.

Testing

ctest -R "bmqsys|bmqimp|mqbnet"

@MorganaFuture MorganaFuture marked this pull request as ready for review October 18, 2025 17:59
@MorganaFuture MorganaFuture requested a review from a team as a code owner October 18, 2025 17:59
@MorganaFuture
Refactor[bmqsys]: Remove bmqsys_threadutil, use bslmt_threadutil instead
cfe98fb 
Remove bmqsys::ThreadUtil wrapper which duplicated bslmt::ThreadUtil
functionality from BDE. Migrate all usages to bslmt::ThreadUtil and
remove redundant files.

Signed-off-by: MorganaFuture <[email protected]>
@MorganaFuture MorganaFuture force-pushed the MorganaFuture/refactor/issue-950-use-bslmt-threadutil branch from 017ceaa to cfe98fb Compare October 19, 2025 09:47
@MorganaFuture
Copy link
Author

@678098 I have added a signature. Could you please rerun CI/CD?

@678098
Copy link
Collaborator

678098 commented Oct 20, 2025

@MorganaFuture thank you for the contribution! I will review the PR
The IT failure is unrelated

FAILED test_admin_client.py::test_app_id_stats[multi_node_fsm-strong_consistency] - RuntimeError: Path: after-confirm.values.queue_consumers_count, 2 != 3 (expected)
======== 1 failed, 186 passed, 1 skipped, 5 rerun in 748.97s (0:12:28) =========

678098
678098 reviewed Oct 20, 2025
View reviewed changes
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.

Thank you for the PR, it is mostly good.
The most important places are with setCurrentThreadOnce, since the behaviour changed there
The rest of the comments are less important style/simplifications

src/groups/bmq/bmqimp/bmqimp_application.cpp
// therefore have to let it stop in the application thread, i.e., from the
// destructor of this object.
bslmt::ThreadAttributes attr = bmqsys::ThreadUtil::defaultAttributes();
bslmt::ThreadAttributes attr = bslmt::ThreadAttributes();
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
bslmt::ThreadAttributes attr = bslmt::ThreadAttributes();
bslmt::ThreadAttributes attr;

Possible to simplify

src/groups/bmq/bmqimp/bmqimp_brokersession.cpp

BALL_LOG_DEBUG << "Adding PUT message for retransmission ["
<< "GUID: '" << builder.messageGUID() << "'] ";
BALL_LOG_DEBUG << "Adding PUT message for retransmission [" << "GUID: '"
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
BALL_LOG_DEBUG << "Adding PUT message for retransmission [" << "GUID: '"
BALL_LOG_DEBUG << "Adding PUT message for retransmission [GUID: '"

Since this line was affected, possible to simplify

src/groups/bmq/bmqimp/bmqimp_brokersession.cpp
// Spawn the FSM thread
bslmt::ThreadAttributes threadAttributes =
bmqsys::ThreadUtil::defaultAttributes();
bslmt::ThreadAttributes threadAttributes = bslmt::ThreadAttributes();
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
bslmt::ThreadAttributes threadAttributes = bslmt::ThreadAttributes();
bslmt::ThreadAttributes threadAttributes;

Possible to simplify

src/groups/bmq/bmqimp/bmqimp_brokersession.cpp
// executed by the *IO* thread

bmqsys::ThreadUtil::setCurrentThreadNameOnce("bmqTCPIO");
bslmt::ThreadUtil::setThreadName("bmqTCPIO");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, the behaviour changed.
Previously the thread name was set only once in each thread.
Now it is changed every time this function is executed.

I would propose to put setCurrentThreadOnce to unnamed namespace in this .cpp file and call it here. This way, we still get rid of bmqsys::ThreadUtil.

void setCurrentThreadNameOnce(const bsl::string& value)
{
#ifdef BSLS_PLATFORM_CMP_CLANG
    // Suppress "exit-time-destructor" warning on Clang by qualifying the
    // static variable 's_named' with Clang-specific attribute.
    [[clang::no_destroy]]
#endif
    static bmqu::TLSBool s_named(false, true);

    if (BSLS_PERFORMANCEHINT_PREDICT_UNLIKELY(!s_named)) {
        BSLS_PERFORMANCEHINT_UNLIKELY_HINT;
        setCurrentThreadName(value);
        s_named = true;
    }
}

src/groups/bmq/bmqimp/bmqimp_eventqueue.cpp

bslmt::ThreadAttributes threadAttributes =
bmqsys::ThreadUtil::defaultAttributes();
bslmt::ThreadAttributes threadAttributes = bslmt::ThreadAttributes();
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
bslmt::ThreadAttributes threadAttributes = bslmt::ThreadAttributes();
bslmt::ThreadAttributes threadAttributes;

Possible to simplify

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp

bslmt::ThreadAttributes attributes =
bmqsys::ThreadUtil::defaultAttributes();
bslmt::ThreadAttributes attributes = bslmt::ThreadAttributes();
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
bslmt::ThreadAttributes attributes = bslmt::ThreadAttributes();
bslmt::ThreadAttributes attributes;

Possible to simplify

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp
BALL_LOG_WARN << "#TCP_UNEXPECTED_STATE "
<< "TCPSessionFactory '" << d_config.name()
<< "' is not listening";
BALL_LOG_WARN << "#TCP_UNEXPECTED_STATE " << "TCPSessionFactory '"
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
BALL_LOG_WARN << "#TCP_UNEXPECTED_STATE " << "TCPSessionFactory '"
BALL_LOG_WARN << "#TCP_UNEXPECTED_STATE TCPSessionFactory '"

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp
Comment on lines +1417 to +1418
BALL_LOG_ERROR << "#TCP_LISTEN_FAILED " << "TCPSessionFactory '"
<< d_config.name() << "' " << "failed listening to '"
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
BALL_LOG_ERROR << "#TCP_LISTEN_FAILED " << "TCPSessionFactory '"
<< d_config.name() << "' " << "failed listening to '"
BALL_LOG_ERROR << "#TCP_LISTEN_FAILED TCPSessionFactory '"
<< d_config.name() << "' failed listening to '"

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp
Comment on lines +1475 to +1476
BALL_LOG_ERROR << "#TCP_CONNECT_FAILED " << "TCPSessionFactory '"
<< d_config.name() << "' " << "failed connecting to '"
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
BALL_LOG_ERROR << "#TCP_CONNECT_FAILED " << "TCPSessionFactory '"
<< d_config.name() << "' " << "failed connecting to '"
BALL_LOG_ERROR << "#TCP_CONNECT_FAILED TCPSessionFactory '"
<< d_config.name() << "' failed connecting to '"

src/groups/mqb/mqbnet/mqbnet_tcpsessionfactory.cpp
// is fine to do this here (since we have no other ways to
// proactively-execute code in the IO threads created by the channelPool).
bmqsys::ThreadUtil::setCurrentThreadNameOnce(d_threadName);
bslmt::ThreadUtil::setThreadName(d_threadName);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, the same comment as with bmqimp_brokersession
The behaviour changed, a good way to solve this is to put a copy of setCurrentThreadNameOnce in the unnamed namespace in this .cpp file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@678098 678098 678098 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@MorganaFuture MorganaFuture

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor: remove bmqsys_threadutil and use bslmt_threadutil instead

2 participants

@MorganaFuture @678098