fix(data): release MemoryDB's SQLite Connector registration via RAII … - #5412
Merged
Conversation
…5411 MemoryDB registered the SQLite Connector during construction but never unregistered it, so the registration outlived every instance and stayed in the process-global SessionFactory singleton (in PocoData) until static teardown. SessionFactory holds each connector by SharedPtr<Connector>. On Windows the module unload order at process exit unloads PocoDataSQLite - which owns the Connector's code and vtable - before PocoData's static SessionFactory is destroyed. ~SessionFactory then releases the still-registered connector's SharedPtr and dispatches its virtual destructor through a vtable in unmapped memory: a deterministic access violation in PocoData.dll on shutdown. SessionFactory::add/remove are refcounted, so a symmetric unregister is safe with several live MemoryDB instances - the connector is removed only when the last one is destroyed. Replace the registerConnector() in prepare() with an RAII member that registers in its constructor and unregisters in its destructor, declared before the Session members so it registers before they are built and releases only after they are destroyed. This also balances the registration when the constructor throws part-way through, which a destructor-only release would miss. Adds a regression test asserting a create/destroy leaves the connector's registration state unchanged.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a Windows shutdown crash by ensuring Poco::Data::SQLite::MemoryDB unregisters the SQLite Connector from the process-global Poco::Data::SessionFactory using RAII, and adds a regression test to validate that a MemoryDB create/destroy leaves connector registration state unchanged.
Changes:
- Introduce
MemoryDB::ConnectorRegistrationRAII member to register/unregister the SQLite connector with correct construction/destruction ordering. - Remove connector registration side-effect from
MemoryDB::prepare(). - Add
MemoryDBTest::testConnectorRegistrationBalanced()and wire it into the SQLite testsuite.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Data/SQLite/testsuite/src/MemoryDBTest.h | Declares a new regression test for connector registration balancing. |
| Data/SQLite/testsuite/src/MemoryDBTest.cpp | Implements the regression test and adds it to the test suite. |
| Data/SQLite/src/MemoryDB.cpp | Adds RAII implementation and removes registration from prepare(). |
| Data/SQLite/include/Poco/Data/SQLite/MemoryDB.h | Adds RAII member and ordering constraints to ensure correct lifecycle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per review: the registration guard swallowed a registerConnector() failure and continued, leaving the Session members built against a connector that is not registered - a broken object presented as constructed. Let it propagate so construction fails with the real cause; callers already handle a throwing MemoryDB constructor. The destructor still swallows unregisterConnector, because a throw from a destructor is not actionable and risks terminate during unwind.
…5411 Per review: the test drains the process-global connector refcount to a clean baseline and restored it with a manual loop at the end. A failing assertion throws before that loop runs, leaving the refcount altered and able to cascade-fail later tests in the same process. Restore it from an RAII object's destructor so the baseline is put back during unwinding regardless of how the test exits.
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.
…#5411
MemoryDB registered the SQLite Connector during construction but never unregistered it, so the registration outlived every instance and stayed in the process-global SessionFactory singleton (in PocoData) until static teardown.
SessionFactory holds each connector by SharedPtr. On Windows the module unload order at process exit unloads PocoDataSQLite - which owns the Connector's code and vtable - before PocoData's static SessionFactory is destroyed. ~SessionFactory then releases the still-registered connector's SharedPtr and dispatches its virtual destructor through a vtable in unmapped memory: a deterministic access violation in PocoData.dll on shutdown.
SessionFactory::add/remove are refcounted, so a symmetric unregister is safe with several live MemoryDB instances - the connector is removed only when the last one is destroyed.
Replace the registerConnector() in prepare() with an RAII member that registers in its constructor and unregisters in its destructor, declared before the Session members so it registers before they are built and releases only after they are destroyed. This also balances the registration when the constructor throws part-way through, which a destructor-only release would miss.
Adds a regression test asserting a create/destroy leaves the connector's registration state unchanged.