Recreate passing tests and file-based and in-memory runQuery #370
Conversation
| # Ignore everything in this directory | ||
| * | ||
| # Except this file | ||
| !.gitignore |
There was a problem hiding this comment.
here will come mechanism of dealing with directories as in DB bench PR
| enableForeignKeys conn = do | ||
| stmt <- Sqlite.prepare conn "PRAGMA foreign_keys = ON;" | ||
| _ <- Sqlite.step stmt | ||
| Sqlite.finalize stmt |
There was a problem hiding this comment.
this is needed for file-based. otherwise :
1) Cardano.Wallet.DB.SqliteCorruption, Check db opening/closing, opening and closing of db works
uncaught exception: SqliteException
SQLite3 returned ErrorBusy while attempting to perform close: unable to close due to unfinalized statements or unfinished backups
| pure (conn, backend) | ||
|
|
||
| createSqliteBackend1 :: Maybe FilePath -> LogFunc -> IO SqlBackend | ||
| createSqliteBackend1 fp logFunc = do |
There was a problem hiding this comment.
this one can be married with createSqliteBackend
| runQuery' = withMVar bigLock . const . runQuery conn | ||
| runQuery' cmd = case fp of | ||
| Nothing -> | ||
| withMVar bigLock $ const $ runQuery backend cmd |
There was a problem hiding this comment.
in-memory - this cannot be used with file-based
| Nothing -> | ||
| withMVar bigLock $ const $ runQuery backend cmd | ||
| _ -> | ||
| withMVar bigLock $ const $ runResourceT $ runNoLoggingT $ withSqlConn (createSqliteBackend1 fp) |
There was a problem hiding this comment.
file-based, this cannot be used with in-memory. Needs other pairs of eyes - at this moment do not see why
|
|
||
| verify dbM dbF | ||
|
|
||
| close conn |
There was a problem hiding this comment.
comment this and add withMaxSuccess 1000 and will probably end up with :
uncaught exception: SqliteException
SQLite3 returned ErrorCan'tOpen while attempting to perform open "backup/test.db".
(after 504 tests and 4 shrinks)
as I did in my laptop.
It suggests we need closing when using file-based Sqlite
| handleChunks chunk = do | ||
| (conn, db) <- fileDBLayer | ||
| forM_ chunk (updateDB db) | ||
| close conn |
There was a problem hiding this comment.
comment this also to encounter open handle sooner
|
Please notice, all tests are green |
| Nothing ) | ||
|
|
||
| withDB inMemoryDBLayer $ | ||
| describe "random operation chunks property when writing to/reading from file" $ do |
There was a problem hiding this comment.
I'd suggest the following:
withDB inMemoryDBLayer $
describe
"random operation chunks property when writing to/reading from \
\file" $ doFor compliance with https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#limit-line-length-to-80-characters
This solves the problem when dealing with the SQLite in-memory vs in-file database. By using 'SqlBackend' and calling close', we do not break the abstraction boundary set by persistent and leave the connection management entirely up to persistent
KtorZ
force-pushed
the
paweljakubas/154/file-based-and-inmemory
branch
from
4916de6 to
b5aeff8
Compare
There was a problem hiding this comment.
LGTM. I've added a couple of changes + refactorings on top, so waiting for @paweljakubas to review back.
|
@KtorZ your commits looks very reasonable. I think we can merge it as it is |
Issue Number
#154
Overview
Comments
This PR checks DB in two modes, ie., two places where db can reside : in-memory and in the file. And it seems they are not the same addressed within persistent, hence different
runQuery'. Secondly, because these are two completely different mechanism (memory vs file system) they are differently serviced when detaching is realized. in-memory detaches gracefully, butfile-basedsolution not so gracefully. It turn out that when we get rid ofclosewe can easily end up with situation where we have too many file handles open and :As evidenced above this happened after 503 successful test of
prop_randomOpChunks(I commentedclose dband addednot (null pairs) ==> withMaxSuccess 1000 $ monadicIO (pure inMemoryDB >>= prop)When
close dbis on the test passes. This was the reason I decided to spend time investigating this issue. This also suggests to me that infile-based modewe need to usecloseand the test is needed, alsorunQuery'workaround. Otherwise, someone with small number of open handles in his/her computer will be subjected to this limitation, as anyone that uses our wallet-backend without interruption for long time (I am thinking here about exchanges).