Repository navigation
fix(Foundation, Util, Prometheus, Zip): fixes found while building poco-osp - #5620
Merged
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Collector teardown can access a destroyed registry and race with export’s virtual dispatch, while Windows process termination still mishandles exited processes with code 259.
3 open issues
What changed in this PR
Addresses issues found while building poco-osp across Foundation, Util, Prometheus, and Zip, with regression tests and compatibility documentation.
Changes:
- Bounds glob backtracking, synchronizes memory-pool counters, and corrects Windows process environment and termination behavior.
- Adjusts configuration-event instantiation, adds collector unregistration, and preserves metadata when copying ZIP entries.
- Adds regression coverage and removes overflow, unchecked-operation, and timing problems from existing tests.
| File | Description |
|---|---|
| Zip/testsuite/src/CompressTest.h | Declares ZIP-copy regression tests. |
| Zip/testsuite/src/CompressTest.cpp | Tests extra fields, attributes, and ZIP64 descriptors. |
| Zip/src/ZipManipulator.cpp | Passes directory metadata to copy operations. |
| Zip/src/ZipFileInfo.cpp | Implements attribute copying. |
| Zip/src/Rename.cpp | Forwards metadata when renaming entries. |
| Zip/src/Keep.cpp | Forwards metadata when retaining entries. |
| Zip/src/Compress.cpp | Preserves attributes and corrects descriptor serialization. |
| Zip/include/Poco/Zip/ZipFileInfo.h | Declares attribute-copying support. |
| Zip/include/Poco/Zip/ZipArchive.h | Adds central-directory entry lookup. |
| Zip/include/Poco/Zip/Rename.h | Stores optional source metadata. |
| Zip/include/Poco/Zip/Keep.h | Stores optional source metadata. |
| Zip/include/Poco/Zip/Compress.h | Extends raw-copy API with metadata. |
| Util/testsuite/src/ConfigurationTestSuite.cpp | Registers configuration-event tests. |
| Util/testsuite/src/ConfigurationEventTest.h | Declares event-instantiation regression coverage. |
| Util/testsuite/src/ConfigurationEventTest.cpp | Tests include order and configuration events. |
| Util/testsuite/Makefile | Builds the new event tests. |
| Util/src/AbstractConfiguration.cpp | Removes explicit string-event instantiation. |
| Util/include/Poco/Util/AbstractConfiguration.h | Allows string-event instantiation at use sites. |
| Prometheus/testsuite/src/RegistryTest.h | Declares registry-lifetime tests. |
| Prometheus/testsuite/src/RegistryTest.cpp | Tests registration, teardown, and concurrent export. |
| Prometheus/testsuite/src/PrometheusTestSuite.cpp | Registers registry tests. |
| Prometheus/testsuite/Makefile | Builds registry tests. |
| Prometheus/src/ThreadPoolCollector.cpp | Unregisters during destruction. |
| Prometheus/src/Registry.cpp | Tracks registrations and clears collector back-references. |
| Prometheus/src/ProcessCollector.cpp | Unregisters during destruction. |
| Prometheus/src/IntGauge.cpp | Unregisters during destruction. |
| Prometheus/src/IntCounter.cpp | Unregisters during destruction. |
| Prometheus/src/Histogram.cpp | Unregisters during destruction. |
| Prometheus/src/Gauge.cpp | Unregisters during destruction. |
| Prometheus/src/Counter.cpp | Unregisters during destruction. |
| Prometheus/src/Collector.cpp | Implements automatic unregistration. |
| Prometheus/include/Poco/Prometheus/Registry.h | Documents registry-lifetime behavior. |
| Prometheus/include/Poco/Prometheus/Collector.h | Adds registry tracking and destructor guidance. |
| Prometheus/include/Poco/Prometheus/CallbackMetric.h | Unregisters during destruction. |
| Foundation/testsuite/src/StringTest.cpp | Uses unsigned benchmark accumulators. |
| Foundation/testsuite/src/SPSCQueueTest.cpp | Uses a 64-bit sum and meaningful assertion. |
| Foundation/testsuite/src/ProcessTest.h | Declares environment and termination tests. |
| Foundation/testsuite/src/ProcessTest.cpp | Tests environment inheritance and post-kill behavior. |
| Foundation/testsuite/src/ProcessRunnerTest.cpp | Waits for PID content and checks directory changes. |
| Foundation/testsuite/src/MemoryPoolTest.h | Declares concurrent observer tests. |
| Foundation/testsuite/src/MemoryPoolTest.cpp | Exercises counters during concurrent allocation. |
| Foundation/testsuite/src/GlobTest.h | Declares backtracking regression tests. |
| Foundation/testsuite/src/GlobTest.cpp | Tests matching behavior and bounded execution time. |
| Foundation/testsuite/src/FileTest.cpp | Checks directory-change and symlink results. |
| Foundation/src/Process_WIN32U.cpp | Merges environments and retains killed-process handles. |
| Foundation/src/Glob.cpp | Backtracks only to the latest asterisk. |
| Foundation/include/Poco/ProcessOptions.h | Updates environment-option documentation. |
| Foundation/include/Poco/Process.h | Documents inheritance and waiting after kill. |
| Foundation/include/Poco/MemoryPool.h | Synchronizes counters and clarifies their units. |
| CHANGELOG | Records environment and collector compatibility changes. |
| AI/MCP/include/Poco/AI/MCP/ProcessClient.h | Updates inherited-environment documentation. |
Review effort: Balanced
Comment on lines
+43
to
+46
| /// A class derived from Collector must call unregister() at the | ||
| /// start of its destructor, before any member that exportTo() | ||
| /// uses is destroyed; otherwise a Registry::exportTo() running | ||
| /// in another thread can use a partially destroyed object. |
Contributor
Author
There was a problem hiding this comment.
We could not reproduce this for the classes of this PR, with clang and GCC ThreadSanitizer, high contention and an exporter that forces the overlap: the vptr store at the entry of the most-derived destructor writes the value the object already has, and every store that changes the vptr comes after unregister() has taken the registry mutex. The checks found two related problems, which are fixed now: a collector registers at the end of the constructor of its concrete class (before, an export could reach an object under construction), and the documentation states the rules for every class in the hierarchy, also for a class derived from Counter or Gauge. testConcurrentExportAndDestruction now forces the overlap.
Comment on lines
+37
to
+38
| Registry* pRegistry = _pRegistry.load(); | ||
| if (pRegistry != nullptr) pRegistry->unregisterCollector(this); |
Contributor
Author
There was a problem hiding this comment.
Confirmed with ThreadSanitizer: it happens when a Registry is destroyed while another thread destroys one of its collectors. That is the general rule that an object must not be destroyed while another thread still uses it, so the Registry and Collector documentation now states it as a precondition instead of adding reference-counted registration state.
Comment on lines
+490
to
+491
| DWORD exitCode = 0; | ||
| if (TerminateProcess(handle.process(), 0) == 0 && (GetExitCodeProcess(handle.process(), &exitCode) == 0 || exitCode == STILL_ACTIVE)) |
Contributor
Author
There was a problem hiding this comment.
Agreed. kill() now treats a process whose handle is signalled as ended, so an exit code of 259 no longer makes it throw; ProcessTest::testKillAfterExitWithCodeStillActive covers it on Windows. tryWait() and isRunning() have the same STILL_ACTIVE ambiguity on main and are left for a separate change.
Glob::match() backtracks only to the last '*' MemoryPool and FastMemoryPool observers lock the mutex FastMemoryPool observers are documented as block counts StringTest benchmarks sum into an unsigned sink SPSCQueueTest sums the throughput values in Int64 FileTest checks the chdir and symlink results ProcessRunnerTest waits until the child PID is written ProcessRunnerTest checks the result of changeDir() ProcessRunnerTest comment uses ASCII only ProcessRunnerTest builds without MSVC warning C4456
launch() adds env to the parent environment on Windows ProcessHandle::wait() works after kill() on Windows AI/MCP ProcessClient documents env as added on every platform
Compress::addFileRaw() keeps extra fields of copied entries ZipManipulator keeps the attributes of copied entries Copied entries get the data descriptor format of their header
matejk
force-pushed
the
5616-foundation-util-prometheus-fixes
branch
from
November 3, 2026 19:38
6028297 to
a3f9731
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5616.
Fixes found while building pocoproject/poco-osp; the issue lists the problems. One commit per library, the breaking
changes in their own; every fix has a test that fails without it. Please use a rebase merge.
Glob::match()backtracks only to the last*;MemoryPoolobservers lock the mutex; on Windows,Process::launch()adds the given variables to the parent environment as on POSIX (PROCESS_CLEAR_ENVIRONMENTpasses only them), and
wait()works afterkill().BasicEvent<const std::string>is instantiated where it is used.Collectorunregisters itself.Breaking changes, in the CHANGELOG: the Windows environment default; classes derived from
Collectorcallunregister()first in their destructor.Tests: Foundation, Util, Prometheus and Zip on macOS, Linux (ASan, UBSan) and Windows MSVC.