Skip to content

Fix: Support setting thread names on more platforms than Linux #702

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

Merged
merged 3 commits into from
Apr 22, 2025

Conversation

pniedzielski
Copy link
Collaborator

This series of patches allows us to set thread names on all platforms that BDE's bslmt::ThreadUtil supports, which currently is Linux, Solaris, Darwin, and Windows.

@pniedzielski pniedzielski added enhancement New feature or request A-Broker Area: C++ Broker labels Apr 16, 2025
@pniedzielski pniedzielski requested a review from hallfox April 16, 2025 17:45
@pniedzielski pniedzielski force-pushed the fix/bdlmt-thread-names branch from b307c85 to 85e05b5 Compare April 16, 2025 18:26
This patch replaces the manual implementation of
`bmqsys::ThreadUtil::setCurrentThreadName` with an implementation using BDE,
which supports more platforms.  For the moment, we keep
`bmqsys::ThreadUtil::k_SUPPORT_THREAD_NAME` only being true on Linux, as there
appear to be other places this constant is used.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
The contracts of `bmqsys::ThreadUtil::setCurrentThreadName` and
`bmqsys::ThreadUtil::setCurrentThreadNameOnce` are wide; they may be called on
any platform, and they have no side effects on platforms where setting a thread
name is unsupported.  This means we do not need to guard calls to them by
checking a constant that is only true on platforms where setting a thread name
is supported.  This patch removes all such guards, which removes all uses of
this constant.

In most cases, the use of this constant is in an `if` statement that obviously
wraps some call to `bmqsys::ThreadUtil::setCurrentThreadName` or
`bmqsys::ThreadUtil::setCurrentThreadNameOnce`.  The only less obvious case is
in `m_bmqbrkr_task`’s mtrap support, where this constant guards a write of the
command `k_MTRAP_SET_THREADNAME` to the mtrap pipe.  The command is handled by
`Task::onControlMessage` in the same file, which in turn calls
`bmqsys::ThreadUtil::setCurrentThreadName`.  Presumably, this is where the `if`
guard should have been, but now it is safe to remove the guard on the write.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
With the previous commit, this constant is no longer used in BlazingMQ.
Because it is not part of our public interface, it is safe to remove this
constant.  This patch removes it.

Signed-off-by: Patrick M. Niedzielski <[email protected]>
@pniedzielski pniedzielski force-pushed the fix/bdlmt-thread-names branch from 85e05b5 to 21f316d Compare April 16, 2025 18:32
@pniedzielski pniedzielski marked this pull request as ready for review April 16, 2025 20:48
@pniedzielski pniedzielski requested a review from a team as a code owner April 16, 2025 20:49
Copy link
Contributor

@chrisbeard chrisbeard left a comment

Choose a reason for hiding this comment

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

Looks good, nice cleanup 👍

@pniedzielski pniedzielski merged commit bcd1d70 into bloomberg:main Apr 22, 2025
29 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Broker Area: C++ Broker enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants