Support thread names on Windows and Integrity#2370
Conversation
WalkthroughThis update introduces support for setting thread names during thread creation on Windows and INTEGRITY platforms in the ACE library. The main implementation occurs in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ACE_OS
participant OS_API
Caller->>ACE_OS: thr_create(..., thr_name)
ACE_OS->>OS_API: Create thread
alt Platform is Windows or INTEGRITY and thr_name provided
ACE_OS->>OS_API: Set thread name using platform API
end
ACE_OS-->>Caller: Return thread handle/result
sequenceDiagram
participant User
participant TAO_Thread_Per_Connection_Handler
participant ACE_Thread_Base
User->>TAO_Thread_Per_Connection_Handler: activate_ch(flags, n_threads)
TAO_Thread_Per_Connection_Handler->>TAO_Thread_Per_Connection_Handler: Generate thread names
TAO_Thread_Per_Connection_Handler->>ACE_Thread_Base: activate(flags, n_threads, ..., thread_names)
ACE_Thread_Base-->>TAO_Thread_Per_Connection_Handler: Result
TAO_Thread_Per_Connection_Handler-->>User: Result
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (21)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
TAO/tao/Thread_Per_Connection_Handler.cpp (3)
26-64: Well-structured implementation for thread name managementThe new
activate_chmethod provides thread naming support for the connection handler, improving debugging and monitoring capabilities. Each thread is assigned a descriptive name that includes its index.Some observations:
- The method properly checks
TAO_debug_levelbefore logging- The implementation carefully handles memory by using ACE containers
- The thread name format is clear and informative
- The proper cast to
static_cast<size_t>is used when accessing vector elementsHowever, there's a potential issue with the thread name buffer initialization:
- ACE_CString buffer = " "; + ACE_CString buffer(10, ' '); // Create a 10-character space stringThe current approach creates a fixed string of spaces that may not be sufficient for all thread indexes. Using the constructor with count and character would be more explicit about the intent.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 33-33: There is an unknown macro here somewhere. Configuration is required. If ACE_TEXT is a macro then please configure it.
(unknownMacro)
43-43: Consider using safer string conversion for thread indicesUsing
ACE_OS::itoawith fixed-size buffer could potentially overflow if dealing with very large thread indices. Consider using a more robust conversion method or ensuring sufficient buffer size.- ACE_OS::itoa (static_cast<ACE_INT32> (i), &buffer[0], 10); + char temp[32]; // Sufficient for any integer + ACE_OS::snprintf(temp, sizeof(temp), "%d", static_cast<ACE_INT32>(i)); + buffer = temp;
39-45: Vector initialization can be optimizedThe vector is created with a size of
n_threadsbut then elements are added withpush_back(), which could cause unnecessary reallocations. Consider directly populating the vector at the correct indices.- ACE_Vector<ACE_CString> names (n_threads); - for (int i = 0; i < n_threads; ++i) - { - ACE_CString buffer = " "; - ACE_OS::itoa (static_cast<ACE_INT32> (i), &buffer[0], 10); - names.push_back ("TAO_Thread_Per_Connection_Handler " + buffer); - } + ACE_Vector<ACE_CString> names; + names.size(n_threads); + for (int i = 0; i < n_threads; ++i) + { + char buffer[32]; + ACE_OS::snprintf(buffer, sizeof(buffer), "%d", static_cast<ACE_INT32>(i)); + names[i] = "TAO_Thread_Per_Connection_Handler " + ACE_CString(buffer); + }ACE/ace/OS_NS_Thread.cpp (1)
3885-3888: Added support for named threads on WindowsThis change implements thread naming support for Windows by using the
SetThreadDescriptionAPI. This is a valuable addition that:
- Improves debugging capabilities
- Makes threads easier to identify in profiling tools
- Maintains consistent behavior with other platforms
The code properly checks for null conditions before setting the thread name.
Note that
SetThreadDescriptionwas added in Windows 10 and might not be available in older Windows versions. Consider adding a backward compatibility check:+ #if defined(_WIN32) && defined(NTDDI_WIN10_RS1) && (NTDDI_VERSION >= NTDDI_WIN10_RS1) if (thr_name && *thr_name && *thr_handle) { SetThreadDescription (*thr_handle, ACE_TEXT_ALWAYS_WCHAR (*thr_name)); } + #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
ACE/ace/OS_NS_Thread.cpp(5 hunks)ACE/ace/OS_NS_Thread.h(1 hunks)ACE/ace/OS_NS_Thread.inl(2 hunks)ACE/ace/Sock_Connect.cpp(1 hunks)ACE/ace/Stack_Trace.cpp(1 hunks)ACE/ace/Thread.cpp(2 hunks)ACE/ace/Thread.h(3 hunks)ACE/ace/Thread.inl(1 hunks)ACE/ace/Thread_Manager.cpp(4 hunks)ACE/ace/Thread_Manager.h(4 hunks)ACE/ace/config-integrity-common.h(1 hunks)TAO/tao/GIOP_Message_Base.cpp(0 hunks)TAO/tao/ORB_Core.cpp(1 hunks)TAO/tao/Stub.inl(1 hunks)TAO/tao/Thread_Per_Connection_Handler.cpp(2 hunks)TAO/tao/Thread_Per_Connection_Handler.h(1 hunks)TAO/tao/Transport.cpp(4 hunks)
💤 Files with no reviewable changes (1)
- TAO/tao/GIOP_Message_Base.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
TAO/tao/Thread_Per_Connection_Handler.cpp (2)
TAO/tao/Thread_Per_Connection_Handler.h (1)
activate_ch(49-49)ACE/ace/OS_NS_stdlib.h (1)
itoa(201-201)
🪛 Cppcheck (2.10-2)
TAO/tao/Thread_Per_Connection_Handler.cpp
[error] 33-33: There is an unknown macro here somewhere. Configuration is required. If ACE_TEXT is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ubuntu-24.04 clang++-16
- GitHub Check: ubuntu-24.04 g++-10 ACE for TAO
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e compact
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: ubuntu-24.04 g++-14
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: VS2022Release64
- GitHub Check: VS2019Release64
- GitHub Check: VS2019Release32
- GitHub Check: VS2022WChar
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2022Debug64
- GitHub Check: VS2019Debug64
- GitHub Check: VS2019WChar
- GitHub Check: VS2019Debug32
- GitHub Check: MINGW64-gcc
- GitHub Check: windows-2019 vs2019
- GitHub Check: macos-13-C++
- GitHub Check: macos-14-C++
🔇 Additional comments (31)
TAO/tao/Stub.inl (1)
42-42: Non-functional documentation tweak: Clarify comment styleThe update refines the comment from the more emphatic “forward queue (??!)” to a concise “forward queue (?)”, improving readability without altering behavior.
ACE/ace/config-integrity-common.h (1)
38-38: Adds necessary ioctl argument type for INTEGRITY platformThis defines the type for the second argument of ioctl system calls as
unsigned long, ensuring consistent type usage for ioctl commands across the INTEGRITY platform.ACE/ace/Sock_Connect.cpp (2)
844-845: Uses the proper platform-specific ioctl type for SIOCGLIFNUM commandThe change from
inttoACE_IOCTL_TYPE_ARG2ensures type consistency with the INTEGRITY platform definition and prevents potential type-related issues.
847-848: Uses the proper platform-specific ioctl type for SIOCGIFNUM commandSimilar to the change above, this ensures type consistency for the SIOCGIFNUM command, using the newly defined
ACE_IOCTL_TYPE_ARG2macro rather than a hard-coded type.TAO/tao/Thread_Per_Connection_Handler.h (1)
49-49:Details
✅ Verification successful
New method for activating connection handler with named threads
This method enhances the thread-per-connection handler to support thread names, which corresponds to the other changes for thread naming support on Windows and INTEGRITY platforms.
🏁 Script executed:
#!/bin/bash # Find implementation of this method to verify it correctly creates thread names rg -A 15 "activate_ch.*long flags, int n_threads" --type cppLength of output: 1078
🏁 Script executed:
#!/bin/bash # Locate the implementation of activate_ch to verify thread naming logic rg -n "Thread_Per_Connection_Handler::activate_ch" -C15 --type cppLength of output: 2335
🏁 Script executed:
#!/bin/bash # Display the full implementation of activate_ch to verify thread naming logic sed -n '27,200p' TAO/tao/Thread_Per_Connection_Handler.cppLength of output: 1744
Thread naming correctly implemented in activate_ch
The
activate_chmethod now constructs per-thread names ("TAO_Thread_Per_Connection_Handler <i>") and passes them via thethread_namesarray toACE_Task_Base::activate, matching the naming support added for Windows and INTEGRITY platforms. Excellent addition!ACE/ace/Thread.inl (1)
79-79: Improved pointer spacing consistencyThe whitespace adjustment in the pointer declaration (
const char **thr_nameinstead ofconst char** thr_name) aligns with similar formatting standardization across multiple thread-related files in this PR.ACE/ace/Thread.cpp (1)
20-20: Standardized pointer spacing in function signatures.The changes apply a consistent pointer declaration style with space between the type and asterisk (
const char *instead ofconst char*), aligning with the project's coding conventions for thread name parameters.Also applies to: 56-56
ACE/ace/Thread.h (1)
91-91: Standardized pointer spacing in thread name parameters.Consistently applied pointer declaration style with space between type and asterisk in thread name parameters, matching the formatting changes across related files.
Also applies to: 115-115, 145-145
ACE/ace/Stack_Trace.cpp (1)
51-52: Improved platform-specific function compilation fordetermine_starting_frame.Added conditional compilation directives to ensure the
determine_starting_framehelper function is only compiled on supported platforms where it's needed. This is a good practice for cross-platform code as it helps avoid potential compilation issues on unsupported platforms.Also applies to: 58-58
ACE/ace/OS_NS_Thread.h (1)
1643-1643: Standardized pointer spacing in thread creation function.Updated the spacing in
thr_nameparameter declaration to match the consistent pointer style used throughout related files. This function is crucial for the PR's thread naming support implementation.TAO/tao/Thread_Per_Connection_Handler.cpp (2)
7-7: Required new inclusion for thread name managementThe added include for the Vector template is necessary to support the new thread naming functionality. This allows for proper container management of thread names.
33-34:Details
✅ Verification successful
Possible macro configuration needed for ACE_TEXT
The static analysis tool indicates that there is an unknown macro
ACE_TEXTbeing used here. While this is likely configured elsewhere in the codebase, it's worth verifying that this macro is properly defined in all build environments.
🏁 Script executed:
#!/bin/bash # Check if ACE_TEXT is defined in ACE headers grep -r "^#define ACE_TEXT" --include="*.h" .Length of output: 10321
ACE_TEXT macro is properly defined in ACE headers
We located multiple
#define ACE_TEXTentries inACE/ace/ace_wchar.h, so theACE_TEXTmacro is already configured and available in all build environments. No further action is needed.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 33-33: There is an unknown macro here somewhere. Configuration is required. If ACE_TEXT is a macro then please configure it.
(unknownMacro)
ACE/ace/OS_NS_Thread.cpp (5)
3458-3458: Consistent pointer spacing in parameter declarationThe parameter declaration has been updated to maintain consistent pointer spacing style with
const char **thr_name. This change is a good practice for code readability and consistency.
3945-3946: Added thread naming support for VxWorksThread naming has been added for VxWorks by passing the thread name to
taskSpawn. This is a good improvement that makes thread identification consistent across platforms.
4032-4034: Added thread naming support for INTEGRITY platformThread naming support for INTEGRITY has been added in the
SetupTaskcall with proper null pointer handling. This is a valuable improvement that enhances cross-platform debugging capabilities.
4042-4044: Added thread naming support for non-178B INTEGRITYThread naming support is also added for standard INTEGRITY (non-178B) in the
CommonCreateTaskcall. This ensures consistent thread naming support across all INTEGRITY variants.
3458-4096: Comprehensive thread naming implementationThe changes add thread naming support across multiple platforms (Windows, VxWorks, INTEGRITY) in a consistent manner. This is an excellent improvement that will significantly enhance debugging and monitoring capabilities.
The implementation properly handles:
- Platform-specific APIs for thread naming
- Null pointer checks to avoid crashes
- Consistent parameter naming across function declarations and implementations
This is a well-designed cross-platform enhancement.
ACE/ace/OS_NS_Thread.inl (2)
2382-2385: Added explicit fallback case for unsupported platformsThe code now properly handles platforms without Windows threads or sigwait by marking the signal set parameter as unused and returning -1 to indicate the unsupported operation.
2849-2857: Improved conditional check for thread name functionThe check now properly requires both
ACE_HAS_THREADSandACE_HAS_VXTHREADSto be defined for the function to proceed, aligning with the broader thread naming enhancements in this PR. This tightens the condition to ensure the function is only used on platforms that fully support it.ACE/ace/Thread_Manager.cpp (4)
580-580: Consistent pointer spacing in parameter declarationUpdated formatting to use consistent pointer spacing for
const char **thr_nameparameter, which aligns with the thread naming improvements across the codebase.
695-695: Consistent pointer spacing in parameter declarationUpdated formatting to use consistent pointer spacing for
const char **thr_nameparameter, maintaining consistent style across thread-related interfaces.
736-736: Consistent pointer spacing in array parameter declarationUpdated formatting to use consistent pointer spacing for
const char *thr_name[]parameter, which improves readability and maintains consistent style with other thread name parameters.
779-779: Consistent pointer spacing in array parameter declarationUpdated formatting to use consistent pointer spacing for
const char *thr_name[]parameter, which maintains consistent style with other thread name parameters throughout the thread management interfaces.ACE/ace/Thread_Manager.h (4)
533-533: Consistent pointer spacing forthr_nameinspawn
Updated thespawnsignature to useconst char **thr_name, aligning pointer spacing with other thread‐creation APIs. This is purely stylistic and has no behavioral impact.
610-610: Consistent pointer spacing forthr_nameinspawn_n(array overload)
Standardized the parameter toconst char *thr_name[], matching the array‐style overload and ensuring uniform formatting across methods. No logic change.
693-693: Consistent pointer spacing forthr_namein secondspawn_noverload
Adjusted the array parameter toconst char *thr_name[]to mirror the otherspawn_noverload and maintain stylistic consistency.
1118-1118: Consistent pointer spacing forthr_namein internalspawn_i
Changed the internalspawn_isignature toconst char **thr_namefor consistency with the publicspawnAPI. Purely formatting.TAO/tao/ORB_Core.cpp (1)
3138-3142: Formatting-only: streamlined switch in get_transport_queueing_strategyThe refactored switch for
TAO_ORB_Core::get_transport_queueing_strategyremoves unnecessary braces around theswitchand itscaseblocks, improving readability without altering any return logic. No behavioral changes detected.TAO/tao/Transport.cpp (3)
2468-2477: Removed unreachable break in CloseConnection caseIn the
case GIOP::CloseConnectionbranch, thereturn -1;now suffices to exit the function, so the redundantbreak;has been removed. This change is purely cleanup—no functional impact.
2482-2492: Streamlined Request case formattingThe
case GIOP::Request/LocateRequestblock has been flattened by removing extra braces and positioning thebreak;directly after the conditional return. This refactoring preserves existing behavior while reducing visual clutter.
2493-2501: Scoped Reply case with explicit bracesThe
case GIOP::Reply/LocateReplyblock now encloses its multi-statement body in braces, making the scope ofTAO_Pluggable_Reply_Params paramsclear. This is a no-op refactor for clarity and does not change functionality.
Updated NEWS
Support thread names on Windows (cherry picked from commit e3b027b) # Conflicts: # ACE/NEWS # ACE/ace/OS_NS_Thread.cpp # ACE/ace/OS_NS_Thread.inl # ACE/ace/Sock_Connect.cpp # ACE/ace/config-integrity-common.h # TAO/NEWS # TAO/tao/ORB_Core.cpp # TAO/tao/Thread_Per_Connection_Handler.cpp
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores