Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make transaction opening more safe #20

Open
Kerollmops opened this issue Oct 28, 2019 · 7 comments
Open

Make transaction opening more safe #20

Kerollmops opened this issue Oct 28, 2019 · 7 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@Kerollmops
Copy link
Member

Kerollmops commented Oct 28, 2019

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 Kerollmops self-assigned this Oct 28, 2019
@Kerollmops Kerollmops added enhancement New feature or request good first issue Good for newcomers labels Oct 28, 2019
@ShadowJonathan
Copy link

How would this work with Send and the likes?

@Kerollmops Kerollmops added this to the v0.20.0 milestone Jan 11, 2023
@Kerollmops
Copy link
Member Author

Related to #148.

@Kerollmops
Copy link
Member Author

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 Kerollmops changed the title Make write transaction opening more safe Make transaction opening more safe Jan 11, 2023
@hyc
Copy link

hyc commented Jan 11, 2023

  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.

@ShadowJonathan
Copy link

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.

@Kerollmops
Copy link
Member Author

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.

@ShadowJonathan
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants