search

verement / lmdb-simple

Simple Haskell API for LMDB
https://hackage.haskell.org/package/lmdb-simple
BSD 3-Clause "New" or "Revised" License
12 stars 11 forks source link

runInBoundThread considered harmful #6

Open infinity0 opened 5 years ago

infinity0 commented 5 years ago

runInBoundThread forks off another thread. However, suppose you're writing an application that manages a bunch of threads. Due to POSIX standards [1] one has to code the main function to catch all exceptions, track child threads, cancel the child threads when you get an exception (which may trigger cleanups in each thread) and then wait for these child threads to exit.

runInBoundThread interferes with this process, since it (a) forks off a worker-bound-thread that nobody else knows about, (b) is uninterruptible while this worker-bound-thread runs, and (c) and if it receives an asynchronous exception in the meantime, it will not rethrow this to the worker-bound-thread. (Arguably the combination of (b) and (c) are design bugs and I'll file a GHC bug shortly.)

In the case of simple-lmdb this is problematic because the worker-bound-thread is often trying to take a lock e.g. in mdb_txn_begin where it calls _lockEnv. If the database has already been closed, then the lock is already taken. Then, the thread will hang forever because it is running inside of runInBoundThread which is uninterruptible.

The simple fix is to use withAsyncBound and wait from Control.Concurrent.Async - define runInBoundThread' action = withAsyncBound action wait and use this instead of runInBoundThread everywhere.

[1] these standards dictate that when the main-thread exits, this also causes all child threads to exit immediately without giving the Haskell runtime a chance to run cleanups

infinity0 commented 5 years ago

See also https://github.com/dmbarbour/haskell-lmdb/issues/4

infinity0 commented 5 years ago

BTW, whether it's legitimate to close the database and then try to perform actions on it, is a separate discussion. My point is that, trying to perform subsequent actions should not result in an indefinite uninterruptible hang, as is the case with the current code. Generally close functions are idempotent or at the very least throw an immediate exception.

dmbarbour commented 5 years ago

Using the LMDB database after closing it will normally result in undefined behavior, likely a crash. An indefinite hang is (arguably) much better.