heed icon indicating copy to clipboard operation
heed copied to clipboard

Make transaction opening more safe

Open Kerollmops opened this issue 4 years ago • 7 comments

Heed could ensure that only one write transaction is ever opened on the same thread.

It can create a thread_local atomic counter for write transactions and raise an error (panic or not) when the user try to open a write transaction and another one is already open.

According to the LMDB documentation, there must never be more than one transaction on the same thread at any time. We could ensure that when we call the read/write_txn function and write into a global variable to check that no other transaction is already opened.

A thread can only use one transaction at a time, plus any child transactions. Each transaction belongs to one thread. See below. The MDB_NOTLS flag changes this for read-only transactions.

Kerollmops avatar Oct 28 '19 20:10 Kerollmops

How would this work with Send and the likes?

ShadowJonathan avatar Jul 08 '21 19:07 ShadowJonathan

Related to #148.

Kerollmops avatar Jan 11 '23 13:01 Kerollmops

Hey @ShadowJonathan,

How would this work with Send and the likes?

Sorry for the delay. This is indeed something that I keep in mind. However, write transactions are non-Send (can't be moved between threads) and non-Sync (can't be referenced from another thread).

I also have some tricky questions for @hyc (👋) about that:

  • Is it allowed to have multiple read/write transactions opened on the same thread if they doesn't refer to the same environment?
  • If MDB_NOTLS is enabled, is it allowed to have one write transaction on a thread and multiple read transactions on this same thread? The documentation explains that the read transaction will be stored in the MDB_txn struct and not a thread-local variable. Therefore I would answer yes to my question.
  • Can you also confirm that when MDB_NOTLS is enabled, we can move the read transaction between threads and also refer to them from other threads?

Kerollmops avatar Jan 11 '23 13:01 Kerollmops

  1. yes. different environments know nothing about each other and don't interact with each other.
  2. yes but it is nonsensical to have multiple read txns on a single thread.
  3. yes but a txn can only be used by one thread at a time, regardless. concurrent use of a single txn from multiple threads will cause memory corruption.

A thread is a single unit of execution. So is a transaction. It is nonsensical to associate them other than 1:1.

hyc avatar Jan 11 '23 14:01 hyc

FWIW that association can easily be broken when async comes in the mix, Send or not, because then a thread can consequently open multiple transactions on the same thread, and work with them on their own dime.

ShadowJonathan avatar Jan 11 '23 15:01 ShadowJonathan

FWIW that association can easily be broken when async comes in the mix, Send or not, because then a thread can consequently open multiple transactions on the same thread, and work with them on their own dime.

Indeed, this is why I would like to add a runtime check to only allow one transaction on a thread, write transactions are neither Send nor Sync. For read transactions, I would like to either always enable MDB_NOTLS, but I don't know if it can bring some limitation with other compile time options or keep track of the number of opened transactions too. Note that if the sync-read-txn heed feature is not enabled then the RoTxn is neither Send nor Sync and then it is safe as it is impossible to do a .await when a transaction is live.

Kerollmops avatar Jan 11 '23 15:01 Kerollmops

Note that if the sync-read-txn heed feature is not enabled then the RoTxn is neither Send nor Sync and then it is safe as it is impossible to do a .await when a transaction is live.

It's not required for a Future to be Send, and there are async executor methods to run them.

ShadowJonathan avatar Jan 11 '23 16:01 ShadowJonathan