IreneKnapp / direct-sqlite

MIT License
35 stars 54 forks source link

Unreliable sqlErrorDetails in parallelized environments #77

Closed kderme closed 5 years ago

kderme commented 6 years ago

When many threads use the same connection, exceptions thrown by some calls like step can be misleading, because of race conditions. This happens because when step gets some error code, it does a followup call on c_sqlite3_errmsg which brings the error message from SQlite. However, it is reported by SQlite that in Serialized mode, this error message can change if between the two calls, some other thread use the same connection https://www.sqlite.org/c3ref/errcode.html (search "serialized threading mode").

This can get critical if applications depend on this error message: for example they expect that a Constraint Violation of a Primary key is thrown, but they get that a Not Null Constraint Violation. Note that the main error (in this case Constraint Violation) is not prone to this race condition, but the additional details are.

I have run the following example and I got all sorts of colorful messages:

import Database.SQLite.Simple

createPeopleTableSQL :: Query
createPeopleTableSQL =
  "CREATE TABLE People (name varchar(50) NOT NULL UNIQUE               \
  \                    ,id   int         NOT NULL UNIQUE              \
  \                    );"

testErrors :: IO ()
testErrors = do
    conn <- open ":memory:"
    execute_ conn createPeopleTableSQL
    _  <- toMsg $ execute conn "INSERT INTO People (name, id) VALUES (?, ?)" ( ("Ann", 0) :: (Text, Int) )
    forkIO $ thread0 conn
    forkIO $ thread1 conn
    threadDelay 100000

thread0 :: Connection -> IO ()
thread0 conn = do
    forM_ [1..10000] $ \n -> do
        result1 <- toMsg $ execute conn "INSERT INTO People (name, id) VALUES (?, ?)" ( ("Ann", n) :: (Text, Int) )
        unless (result1 == "SQLite3 returned ErrorConstraint while attempting to perform step: UNIQUE constraint failed: People.name")
            (error $ ("thread 0 trial " <> show n <> " got: " :: String) <> result1)

thread1 :: Connection -> IO ()
thread1 conn = do
    forM_ [1..10000] $ \n -> do
        _ <- toMsg $ execute conn "INSERT INTO People (name, id) VALUES (?, ?)" ( (show n, 0) :: (Text, Int) )
        return ()

toMsg :: IO () -> IO String
toMsg action = do
    res <- try action
    case res of
        Left (e :: SomeException)  -> return $ displayException e
        Right _ -> return $ "no error"

Some results I got:

In first result note that the message UNIQUE constraint failed: People.id is what thread1 excepts, as it adds an entry with the same id, which should be Unique. But because of the race condition this result is thrown on thread0!

I understand this is not exactly a bug of this library, but a strange behaviour of SQlite. SQlite suggests the use of sqlite3_mutex_enter(), if we want to have reliably the correct message (sqlite-direct does not have bindings for this one) or to use the extended error codes. Are there any plans to add any of those?

parsonsmatt commented 6 years ago

The imports in that code snippet point to sqlite-simple, not sqlite-direct.

kderme commented 6 years ago

This helps keeping the example short, as direct-sqlite is pretty low level. Keep in mind sqlite-simple is built on top of direct-sqlite. execute from sqlite-simple is just a bracket around prepate-bind-step-close. I opened the issue here because sqlite-simple can't solve the issue without changes in the bindings.

moll commented 5 years ago

Why did you close the issue, @kderme?

nurpax commented 5 years ago

A long time ago when I hit a related problem (writing and reading from multiple threads, leading to busy errors), I fixed this by mutexing all sqlite operations with an MVar. IMO this is a much better solution than trying to work-around this at lower levels. Even if you manage to fix one problem by using the sqlite api differently, most likely you will hit many more problems if you're using sqlite from multiple threads on the same connection.

That'd definitely be my recommendation: guard sqlite operations with MVars. If that doesn't scale for your application (e.g., you need better concurrency, performance), use something else than sqlite.

kderme commented 5 years ago

Indeed, I found that there are more issues when using the same connection from multiple threads. Actually I am writing a blog-post about it, as part of my GSoC project here https://github.com/kderme/gsoc/blob/master/blog/blog-sqlite.md (feedback is very welcome)

There, I propose that there is a singe thread which does all the writing and waits for insert commands from other threads in a queue. Reads can be done safely and concurrently by multiple threads when they don't share the same connections.