From c524a6ae7bb9e5cf127a46ff65870c3cafb1de28 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Mon, 16 Dec 2019 14:06:54 +0100 Subject: [PATCH 1/2] make sure to close and destroy the database before removing files from disk --- lib/core/src/Cardano/Wallet/Api/Server.hs | 4 +- lib/core/src/Cardano/Wallet/DB/Sqlite.hs | 69 ++++++++++--------- .../test/unit/Cardano/Wallet/DB/SqliteSpec.hs | 2 +- .../src/Cardano/Wallet/Jormungandr.hs | 18 +---- 4 files changed, 43 insertions(+), 50 deletions(-) diff --git a/lib/core/src/Cardano/Wallet/Api/Server.hs b/lib/core/src/Cardano/Wallet/Api/Server.hs index ea0f6656e91..ce24ec7c7c7 100644 --- a/lib/core/src/Cardano/Wallet/Api/Server.hs +++ b/lib/core/src/Cardano/Wallet/Api/Server.hs @@ -481,9 +481,9 @@ deleteWallet -> ApiT WalletId -> Handler NoContent deleteWallet ctx (ApiT wid) = do - liftHandler $ withWorkerCtx ctx wid throwE $ \wrk -> W.deleteWallet wrk wid - liftIO $ Registry.remove re wid + liftHandler $ withWorkerCtx @_ @s @k ctx wid throwE $ \_ -> pure () liftIO $ (df ^. #removeDatabase) wid + liftIO $ Registry.remove re wid return NoContent where re = ctx ^. workerRegistry @s @k diff --git a/lib/core/src/Cardano/Wallet/DB/Sqlite.hs b/lib/core/src/Cardano/Wallet/DB/Sqlite.hs index b4f473d5964..6fc72584a8c 100644 --- a/lib/core/src/Cardano/Wallet/DB/Sqlite.hs +++ b/lib/core/src/Cardano/Wallet/DB/Sqlite.hs @@ -23,7 +23,7 @@ module Cardano.Wallet.DB.Sqlite ( newDBLayer - , mkDBFactory + , newDBFactory , findDatabases , withDBLayer @@ -91,6 +91,8 @@ import Cardano.Wallet.Primitive.AddressDiscovery ( IsOurs (..) ) import Control.Arrow ( (***) ) +import Control.Concurrent.MVar + ( newEmptyMVar, putMVar, readMVar ) import Control.DeepSeq ( NFData ) import Control.Exception @@ -190,17 +192,17 @@ withDBLayer -- ^ Logging object -> Maybe FilePath -- ^ Path to database directory, or Nothing for in-memory database - -> (DBLayer IO s k -> IO a) + -> ((SqliteContext, DBLayer IO s k) -> IO a) -- ^ Action to run. -> IO a -withDBLayer logConfig trace mDatabaseDir action = - bracket before after (action . snd) +withDBLayer logConfig trace mDatabaseDir = + bracket before after where before = newDBLayer logConfig trace mDatabaseDir after = destroyDBLayer . fst -- | Instantiate a 'DBFactory' from a given directory -mkDBFactory +newDBFactory :: forall s k. ( IsOurs s W.Address , IsOurs s W.ChimericAccount @@ -216,33 +218,38 @@ mkDBFactory -- ^ Logging object -> Maybe FilePath -- ^ Path to database directory, or Nothing for in-memory database - -> DBFactory IO s k -mkDBFactory cfg tr mDatabaseDir = case mDatabaseDir of - Nothing -> DBFactory - { withDatabase = \_ -> - withDBLayer cfg tracerDB Nothing - , removeDatabase = \_ -> - pure () - } - Just databaseDir -> DBFactory - { withDatabase = \wid -> - withDBLayer cfg tracerDB (Just $ databaseFile wid) - , removeDatabase = \wid -> do - let files = - [ databaseFile wid - , databaseFile wid <> "-wal" - , databaseFile wid <> "-shm" - ] - mapM_ removePathForcibly files - } + -> IO (DBFactory IO s k) +newDBFactory cfg tr mDatabaseDir = do + mvar <- newEmptyMVar + case mDatabaseDir of + Nothing -> pure DBFactory + { withDatabase = \_ action -> + withDBLayer cfg tracerDB Nothing (action . snd) + , removeDatabase = \_ -> + pure () + } + Just databaseDir -> pure DBFactory + { withDatabase = \wid action -> + withDBLayer cfg tracerDB (Just $ databaseFile wid) $ \(ctx,db) -> do + putMVar mvar ctx + action db + , removeDatabase = \wid -> do + let files = + [ databaseFile wid + , databaseFile wid <> "-wal" + , databaseFile wid <> "-shm" + ] + readMVar mvar >>= destroyDBLayer + mapM_ removePathForcibly files + } + where + databaseFilePrefix = keyTypeDescriptor $ Proxy @k + databaseFile wid = + databaseDir + databaseFilePrefix <> "." <> + T.unpack (toText wid) <> ".sqlite" where - databaseFilePrefix = keyTypeDescriptor $ Proxy @k - databaseFile wid = - databaseDir - databaseFilePrefix <> "." <> - T.unpack (toText wid) <> ".sqlite" - where - tracerDB = appendName "database" tr + tracerDB = appendName "database" tr -- | Return all wallet databases that match the specified key type within the -- specified directory. diff --git a/lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs b/lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs index e078daa8278..9d861d071b5 100644 --- a/lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs +++ b/lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs @@ -511,7 +511,7 @@ withTestDBFile action expectations = do withSystemTempFile "spec.db" $ \fp handle -> do hClose handle removeFile fp - withDBLayer logConfig trace (Just fp) action + withDBLayer logConfig trace (Just fp) (action . snd) expectations fp inMemoryDBLayer diff --git a/lib/jormungandr/src/Cardano/Wallet/Jormungandr.hs b/lib/jormungandr/src/Cardano/Wallet/Jormungandr.hs index c9ee5da80ff..1d7cd030612 100644 --- a/lib/jormungandr/src/Cardano/Wallet/Jormungandr.hs +++ b/lib/jormungandr/src/Cardano/Wallet/Jormungandr.hs @@ -51,8 +51,6 @@ import Cardano.Wallet.Api.Types ( DecodeAddress, EncodeAddress ) import Cardano.Wallet.DaedalusIPC ( daedalusIPC ) -import Cardano.Wallet.DB - ( DBFactory ) import Cardano.Wallet.DB.Sqlite ( PersistState ) import Cardano.Wallet.Jormungandr.Compatibility @@ -236,8 +234,9 @@ serveWallet apiLayer tracer tl nl = do let (block0, bp) = staticBlockchainParameters nl wallets <- maybe (pure []) (Sqlite.findDatabases @k trText) databaseDir + db <- Sqlite.newDBFactory cfg trText databaseDir Server.newApiLayer - tracer (toWLBlock block0, bp, sTolerance) nl' tl dbFactory wallets + tracer (toWLBlock block0, bp, sTolerance) nl' tl db wallets where nl' = toWLBlock <$> nl @@ -256,19 +255,6 @@ serveWallet onExit = defaultWorkerAfter tr' trStakePool = contramap (fmap LogStakePoolLayerMsg) trRoot - dbFactory - :: forall s k. - ( IsOurs s Address - , IsOurs s ChimericAccount - , NFData s - , Show s - , PersistState s - , PersistPrivateKey (k 'RootK) - , WalletKey k - ) - => DBFactory IO s k - dbFactory = Sqlite.mkDBFactory cfg trText databaseDir - handleNetworkStartupError :: ErrStartup -> IO ExitCode handleNetworkStartupError = \case ErrStartupGetBlockchainParameters e -> case e of From 41798077422dc428b800242af46dd4222c1a6c59 Mon Sep 17 00:00:00 2001 From: KtorZ Date: Mon, 16 Dec 2019 15:07:12 +0100 Subject: [PATCH 2/2] add unit test for the DBFactory, testing that open/clear works correctly --- lib/core/src/Cardano/DB/Sqlite.hs | 11 +++++- .../test/unit/Cardano/Wallet/DB/SqliteSpec.hs | 35 ++++++++++++++++--- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/lib/core/src/Cardano/DB/Sqlite.hs b/lib/core/src/Cardano/DB/Sqlite.hs index 50a42fb9278..41882ecc9f4 100644 --- a/lib/core/src/Cardano/DB/Sqlite.hs +++ b/lib/core/src/Cardano/DB/Sqlite.hs @@ -44,6 +44,8 @@ import Cardano.Wallet.Logging ( logTrace ) import Control.Concurrent.MVar ( newMVar, withMVar ) +import Control.Exception + ( throwIO ) import Control.Monad ( mapM_ ) import Control.Monad.Catch @@ -129,7 +131,14 @@ handleConstraint e = handleJust select handler . fmap Right destroyDBLayer :: SqliteContext -> IO () destroyDBLayer (SqliteContext {getSqlBackend, trace, dbFile}) = do logDebug trace (MsgClosing dbFile) - close' getSqlBackend + tryClose + where + tryClose = close' getSqlBackend + `catch` handleBusy + + handleBusy e@(SqliteException name _ _) = case name of + Sqlite.ErrorBusy -> tryClose + _ -> throwIO e {------------------------------------------------------------------------------- Internal / Database Setup diff --git a/lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs b/lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs index 9d861d071b5..f58961a9fe2 100644 --- a/lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs +++ b/lib/core/test/unit/Cardano/Wallet/DB/SqliteSpec.hs @@ -51,13 +51,18 @@ import Cardano.Crypto.Wallet import Cardano.DB.Sqlite ( SqliteContext, destroyDBLayer ) import Cardano.Wallet.DB - ( DBLayer (..), ErrNoSuchWallet (..), PrimaryKey (..), cleanDB ) + ( DBFactory (..) + , DBLayer (..) + , ErrNoSuchWallet (..) + , PrimaryKey (..) + , cleanDB + ) import Cardano.Wallet.DB.Arbitrary ( KeyValPairs (..) ) import Cardano.Wallet.DB.Properties ( properties, withDB ) import Cardano.Wallet.DB.Sqlite - ( PersistState, newDBLayer, withDBLayer ) + ( PersistState, newDBFactory, newDBLayer, withDBLayer ) import Cardano.Wallet.DB.StateMachine ( prop_parallel, prop_sequential ) import Cardano.Wallet.DummyTarget.Primitive.Types @@ -106,12 +111,14 @@ import Cardano.Wallet.Primitive.Types ) import Cardano.Wallet.Unsafe ( unsafeRunExceptT ) +import Control.Concurrent + ( forkIO ) import Control.DeepSeq ( NFData ) import Control.Exception ( throwIO ) import Control.Monad - ( forM_, replicateM_, unless ) + ( forM_, forever, replicateM_, unless ) import Control.Monad.IO.Class ( liftIO ) import Control.Monad.Trans.Except @@ -137,13 +144,13 @@ import Data.Time.Clock import GHC.Conc ( TVar, newTVarIO, readTVarIO, writeTVar ) import System.Directory - ( doesFileExist, removeFile ) + ( doesFileExist, listDirectory, removeFile ) import System.IO ( hClose ) import System.IO.Error ( isUserError ) import System.IO.Temp - ( emptySystemTempFile, withSystemTempFile ) + ( emptySystemTempFile, withSystemTempDirectory, withSystemTempFile ) import System.IO.Unsafe ( unsafePerformIO ) import System.Random @@ -174,6 +181,7 @@ import qualified Cardano.BM.Data.AggregatedKind as CM import qualified Cardano.BM.Data.Backend as CM import qualified Cardano.BM.Data.SubTrace as CM import qualified Cardano.Wallet.Primitive.AddressDerivation.Shelley as Seq +import qualified Data.ByteString.Char8 as B8 import qualified Data.HashMap.Strict as HM import qualified Data.List as L import qualified Data.Set as Set @@ -343,6 +351,23 @@ fileModeSpec = do (ctx, _) <- newDBLayer' @(SeqState 'Testnet ShelleyKey) db destroyDBLayer ctx + describe "DBFactory" $ do + it "withDatabase *> removeDatabase works and remove files" $ do + withSystemTempDirectory "DBFactory" $ \dir -> do + cfg <- defaultConfigTesting + DBFactory{..} <- newDBFactory cfg nullTracer (Just dir) + + -- NOTE + -- Start a concurrent worker which makes action on the DB in + -- parallel to simulate activity. + _ <- forkIO $ withDatabase testWid $ \(DBLayer{..} :: TestDBSeq) -> do + forever $ do + cp <- atomically $ readCheckpoint $ PrimaryKey testWid + B8.putStrLn (B8.pack $ show cp) + + removeDatabase testWid + listDirectory dir `shouldReturn` mempty + describe "Sqlite database file" $ do let writeSomething DBLayer{..} = do atomically $ unsafeRunExceptT $