Skip to content

Feature native thread info#5135

Merged
aleks-f merged 18 commits intomainfrom
feature-NativeThreadInfo
Dec 23, 2025
Merged

Feature native thread info#5135
aleks-f merged 18 commits intomainfrom
feature-NativeThreadInfo

Conversation

@aleks-f
Copy link
Copy Markdown
Member

@aleks-f aleks-f commented Dec 22, 2025

Fix Message initialization to correctly handle thread IDs for both POCO
and non-POCO threads. When Thread::current() returns null (std::thread,
main thread, etc.), use NativeThreadInfo to get the OS thread ID instead
of leaving _tid as zero.

Rename NativeThreadInfo::id() to NativeThreadInfo::osTid() to match the
Thread::currentOsTid() naming convention and clarify that both return
the same kernel-level thread identifier (not the POCO sequential ID).

Additional changes:
- Add comprehensive documentation to NativeThreadInfo explaining its
  purpose and when to use it vs Thread::current()
- Fix LoggerTest warning by removing pessimizing std::move in return
- Update all platform implementations (POSIX, WIN32, WINCE, VxWorks)

This ensures the %I format specifier in PatternFormatter shows:
- POCO thread ID (1, 2, 3...) for Poco::Thread instances
- OS thread ID for non-POCO threads (std::thread, main thread, etc.)
testFormatThreadName was failing in CI because it expected the POCO
thread ID to be 1, but in a full test run, many threads are created
before this test (by ThreadingTestSuite, etc.), resulting in higher IDs.

The test now compares against the actual thread's ID (thr.id()) instead
of a hard-coded value, making it robust regardless of test order.
@aleks-f aleks-f self-assigned this Dec 23, 2025
@aleks-f aleks-f added this to the Release 1.15.0 milestone Dec 23, 2025
@aleks-f aleks-f requested review from Copilot and matejk December 23, 2025 01:31

This comment was marked as outdated.

- Thread_WIN32: Use MultiByteToWideChar with CP_UTF8 in setCurrentNameImpl
  instead of byte-by-byte iterator copy. The old conversion was incorrect
  for non-ASCII characters (e.g., Japanese thread names).

- LoggerTest: Add bounds check (parts.size() >= 5) before accessing vector
  elements to prevent undefined behavior if log message format is unexpected.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Move POCO_NO_THREADNAME guard inside inline implementations instead of
around declarations. This ensures the API is always available on all
platforms, returning empty string or no-op on platforms without thread
name support (e.g., emscripten, AIX).

Update Message.cpp to remove conditional compilation since the functions
are now unconditionally available.
Thread_WIN32.cpp:
- Use dynamic loading for SetThreadDescription/GetThreadDescription
  via GetProcAddress for compatibility with pre-Windows 10 1607
- setCurrentNameImpl tries modern API first, falls back to legacy
  exception method (0x406D1388) for older Windows/debuggers
- Remove redundant processthreadsapi.h and stringapiset.h includes
  (already included via windows.h)

LoggerTest.cpp:
- Simplify loop condition (remove redundant npos check)
- Fix code style (braces on new lines)
Copy link
Copy Markdown
Contributor

@matejk matejk left a comment

Choose a reason for hiding this comment

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

The PR is (mostly) platform-specific thread name. PR title shall reflect that.

I have just one minor comment on the PR.

@aleks-f aleks-f merged commit dcaa15a into main Dec 23, 2025
90 checks passed
@aleks-f aleks-f deleted the feature-NativeThreadInfo branch December 23, 2025 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants