fnc12 / sqlite_orm

❤️ SQLite ORM light header only library for modern C++
GNU Affero General Public License v3.0
2.26k stars 313 forks source link

make_backup_to fails when using initStorage #400

Closed unclevlad closed 4 years ago

unclevlad commented 4 years ago
inline auto initStorageMarvel( const std::string& path ) {
    using namespace sqlite_orm;
    auto storage = make_storage(path,
                                make_table("marvel",
                                           make_column("id", &MarvelHero::id, primary_key()),
                                           make_column("name", &MarvelHero::name),
                                           make_column("abilities", &MarvelHero::abilities)));
    return std::move( storage );
}

using MarvelStorage= decltype( initStorageMarvel( "" ) );

TEST_CASE("BackupRestoreTest", "[SqliteOrmVerification]") {
    using namespace sqlite_orm;

    // --- Create a shared pointer to the MarvelStorage
    std::string fp( "iteration.sqlite" );
    std::shared_ptr< MarvelStorage > db = std::make_shared< MarvelStorage >( initStorageMarvel( fp ) );
    auto storage{ *db };

    storage.sync_schema( );
    storage.remove_all<MarvelHero>( );

    // --- Insert values
    storage.insert(MarvelHero{-1, "Tony Stark", "Iron man, playboy, billionaire, philanthropist"});
    storage.insert(MarvelHero{-1, "Thor", "Storm god"});
    storage.insert(MarvelHero{-1, "Vision", "Min Stone"});
    storage.insert(MarvelHero{-1, "Captain America", "Vibranium shield"});
    storage.insert(MarvelHero{-1, "Hulk", "Strength"});
    storage.insert(MarvelHero{-1, "Star Lord", "Humor"});
    storage.insert(MarvelHero{-1, "Peter Parker", "Spiderman"});
    storage.insert(MarvelHero{-1, "Clint Barton", "Hawkeye"});
    storage.insert(MarvelHero{-1, "Natasha Romanoff", "Black widow"});
    storage.insert(MarvelHero{-1, "Groot", "I am Groot!"});
    REQUIRE( storage.count<MarvelHero>( ) == 10 );

    // --- Create backup file name and verify that the file does not exist
    std::string backupFilename{ "backup.sqlite" };

    // --- Backup the current storage to the file
    auto backup = storage.make_backup_to(backupFilename);
    backup.step( -1 );
}

Exceptions thrown when we hit make_backup_to

unclevlad commented 4 years ago

Tried building the existing backup test using Visual Studio 2017, 64-bit, C++17 enabled. It trows exceptions in the same place.

fnc12 commented 4 years ago

One thing we missed when we added CI update - we removed examples compilation. Thank you again. I shall fix it soon

fnc12 commented 4 years ago

Fix is on its way https://github.com/fnc12/sqlite_orm/pull/402

fnc12 commented 4 years ago

bug is fixed. Please checkout dev branch

unclevlad commented 4 years ago

backup_tests.cpp still seg faults on Windows on line 33.

unclevlad commented 4 years ago

Ok, changing both files used in backup_tests.cpp to file-name based rather than memory lets it run until the call to storage,backup_to before throwing an exception in sqliteormtests.exe!sqlite_orm::internal::connection_holder::retain() Line 6492

this->_retain_count is nullptr

unclevlad commented 4 years ago

All of the other tests pass without issue in the same environment.

fnc12 commented 4 years ago

I have just reran your example from this thread and it worked correctly. This is my code:

TEST_CASE("Backup crash") {
    using namespace BackupTests;
    using MarvelStorage = decltype( initStorageMarvel( "" ) );

    // --- Create a shared pointer to the MarvelStorage
    std::string fp( "iteration.sqlite" );
    std::shared_ptr< MarvelStorage > db = std::make_shared< MarvelStorage >( initStorageMarvel( fp ) );
    auto storage{ *db };

    storage.sync_schema( );
    storage.remove_all<MarvelHero>( );

    // --- Insert values
    storage.insert(MarvelHero{-1, "Tony Stark", "Iron man, playboy, billionaire, philanthropist"});
    storage.insert(MarvelHero{-1, "Thor", "Storm god"});
    storage.insert(MarvelHero{-1, "Vision", "Min Stone"});
    storage.insert(MarvelHero{-1, "Captain America", "Vibranium shield"});
    storage.insert(MarvelHero{-1, "Hulk", "Strength"});
    storage.insert(MarvelHero{-1, "Star Lord", "Humor"});
    storage.insert(MarvelHero{-1, "Peter Parker", "Spiderman"});
    storage.insert(MarvelHero{-1, "Clint Barton", "Hawkeye"});
    storage.insert(MarvelHero{-1, "Natasha Romanoff", "Black widow"});
    storage.insert(MarvelHero{-1, "Groot", "I am Groot!"});
    REQUIRE( storage.count<MarvelHero>( ) == 10 );

    // --- Create backup file name and verify that the file does not exist
    std::string backupFilename{ "backup.sqlite" };

    // --- Backup the current storage to the file
    auto backup = storage.make_backup_to(backupFilename);
    backup.step( -1 );
}

Did you pull dev branch?

unclevlad commented 4 years ago

Everything is up to date. Re-ran the same example, hits line auto backup = storage.make_backup_to(backupFilename); Exception thrown at sqlite_orm,.h line#6492 because 'this' is a nullptr

unclevlad commented 4 years ago

Visual Studio 2017, 64-bit build

fnc12 commented 4 years ago

@unclevlad switch please to bugfix/400 branch and run tests target. I've created a test with code you provided here. Tell me please whether it runs ok or fails. Thanks

unclevlad commented 4 years ago

Fails on first call to backup_to( ).

Exception still thrown from sqlite_orm::internal::connection_holder::retain() Line 6492

fnc12 commented 4 years ago

this is weird. I added check in my local branch that if(this == nullptr) throw ... in sqlite_orm::internal::connection_holder::retain() and it never fired. The only difference is that I use macOS with Xcode and you use win with vs

unclevlad commented 4 years ago

Trust me, I am not having fun trying to debug this at my end. I think Windows is doing something naughty under the hood with the pointer.

fnc12 commented 4 years ago

@unclevlad ok let's see it here https://github.com/fnc12/sqlite_orm/pull/404

unclevlad commented 4 years ago

So pull down the branch and try again?

On Sun, Oct 20, 2019 at 3:41 PM Yevgeniy Zakharov notifications@github.com wrote:

@unclevlad https://github.com/unclevlad ok let's see it here #404 https://github.com/fnc12/sqlite_orm/pull/404

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fnc12/sqlite_orm/issues/400?email_source=notifications&email_token=ACVJMCWIS7P356OCPSA3FALQPS7A5A5CNFSM4JBZVO62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBYTOCQ#issuecomment-544290570, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACVJMCSJNWHVHFWCGXGBOLTQPS7A5ANCNFSM4JBZVO6Q .

fnc12 commented 4 years ago

https://github.com/fnc12/sqlite_orm/pull/404 everything worked fine on CI. Try to clean the project locally

unclevlad commented 4 years ago

Ok, so this builds, but incorporating it into our build environment crashes. Thanks. I will need to reduce our env until I find the problem.

On Sun, Oct 20, 2019 at 4:15 PM Yevgeniy Zakharov notifications@github.com wrote:

404 https://github.com/fnc12/sqlite_orm/pull/404 everything worked

fine on CI. Try to clean the project locally

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fnc12/sqlite_orm/issues/400?email_source=notifications&email_token=ACVJMCSMSDVNCA2OZIF6HJTQPTC7LA5CNFSM4JBZVO62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBYUFMY#issuecomment-544293555, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACVJMCUEFDMFZITTM5Z7LXDQPTC7LANCNFSM4JBZVO6Q .