Add a default lock mode to the EntityManager

[BenMorel]

Following this discussion on the mailing list, this proposal introduces a default lock mode for all entities loaded through an EntityManager.

At the moment, there is no way to set a lock mode for the following use cases:

• Proxies obtained through getReference() and then initialized
• Entities lazy-loaded through traversal of associations

This proposal introduces the idea of a default lock mode, which can be set at runtime when all reads in a transaction should be locking.

It works this way:

$transaction = $em->createTransaction()
    ->withDefaultLockMode(LockMode::PESSIMISTIC_WRITE)
    ->begin();

// load entities from EntityManager, Repositories or DQL, traverse associations, etc.
// all these entities will be loaded with the given lock mode

$transaction->commit();

I have successfully tested it with the following use cases:

• EntityManager::find()
• EntityRepository::findBy()
• DQL queries
• Proxies
• Lazy-loaded collections through OneToMany and ManyToMany associations

Happily waiting for your feedback!

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-2973

We use Jira to track the state of pull requests and the versions they got
included in.

[BenMorel]

This is always a Query as far as I can see in the code, and the passing tests seem to confirm that.

[deeky666]

If that is really true, can you please remove the false check in this line? I added that with my last modification because I was not sure whether it always is Query and $this->query->getHint(Query::HINT_LOCK_MODE) which you replaced could return false.

[BenMorel]

No problem, as soon as it's confirmed by one of the lead developers!

[Ocramius]

Hey @BenMorel,

Sorry for not giving you feedback in time.

The patch looks simple and clean, and it is a nice feature addition for the amount of code that it carries.

Do you think that it would be possible to add functional tests for it?
I'd suggest adding tests that would make queries fail and cause exceptions, since those would probably be the simplest ones to implement.

[BenMorel]

Hi @Ocramius, thanks for your reply!

I have added a bunch of tests to cover all the use cases mentioned above:

• EntityManager::find()
• EntityRepository::findBy()
• DQL queries
• Proxies
• Lazy-loaded collections

They all test that the generated SQL query ends (or not) with the FOR UPDATE lock hint.

Note: I had to add functionality to existing mocks, but without breaking any existing tests.

[BenMorel]

One more thing, I'm still not sure about the $em->getConfiguration()->setDefaultLockMode() way.

As explained above, it feels a tiny bit wrong as I think the configuration should be kept for things that are not likely to change after bootstrapping, whereas the default lock mode is something that will change from controller to controller within the same application.

So I would like to suggest two different approaches:

• Setting the default lock mode on the EntityManager directly:
  $em->setDefaultLockMode($lockMode)
• Setting the default lock mode when starting the transaction:
  $em->beginTransaction($lockMode = null)

I quite like the second approach, as it does not make sense (correct me if I'm wrong) to set a lock mode outside a transaction; so we could set a default lock mode that is valid only for the current transaction, and without breaking the existing API. It feels more intuitive this way.

It would be nice to get the opinion of @beberlei, @FabioBatSilva, @guilhermeblanco, @jwage, @asm89, @stof on this point?

[beberlei]

I am against this kind of "global" state. It can lead to very confusing side effects. I would rather have a closure that wraps everything in this kind of lock mode and then disables after commit.

This is something for the transaction object that @guilhermeblanco is always talking about :)

[BenMorel]

@beberlei Thanks for your comment, but what about the third approach then:

$em->beginTransaction($defaultLockMode = null)

It's possible straight away even without a transaction object, does not break the API, and would only be valid for this transaction!

I'm happy to update the code with a proof of concept if you wish to see it implemented.

[guilhermeblanco]

@BenMorel I would be very happy.
Remember that Transaction would then hold pretty much scheduled operations around entities.
This is a medium-hard effort task, which I dunno if I can easily explain what is involved. =
We can spend some time talking about it tomorrow if you want.

[BenMorel]

@guilhermeblanco I would be happy to help further with the transaction object, but to be honest I don't think I have enough knowledge on the subject to be able to start any work on it right now; I was just proposing to move the default lock mode setting to beginTransaction() for now, and hopefully that could then trigger more discussion regarding the transaction object.

That being said, I'm happy to discuss this with you further tomorrow!

[BenMorel]

Ok I've played a bit more with it, and it looks like it will be a little bit hacky to do it that way, and indeed @beberlei is right, this would fit the idea of transaction objects very well.

I would like to give transaction objects a go, and reading a little bit about them, I think we could start with something really simple, that would just encapsulate the commit() and rollback() methods, as well as some transaction-specific configuration such as the default lock mode suggested here.

I'm happy to open another PR with a proof of concept on the DBAL first - and leave this PR on hold for now -, but I need to confirm first with @beberlei that it would be acceptable to:

• Change the Connection::beginTransaction() return type from void to Transaction
• Change the EntityManager::beginTransaction() return type from void to Transaction
• Deprecate Connection::commit() in favour of Transaction::commit()
  This would internally call getCurrentTransaction()->commit()
• Deprecate Connection::rollback() in favour of Transaction::rollback()
  This would internally call getCurrentTransaction()->rollback()

I know that you're meticulously following semver and I don't know how that would translate in terms of bumping versions.

I truly hope that it could fit the next minor version, as this would potentially break no project using Doctrine as is, unless the project uses a custom Connection subclass, but I can't see any real life use case for this (I might be wrong).

Please let me know what you think, I'd be happy to give it a go ASAP!

[BenMorel]

@beberlei I'm actually already seeing inconsistencies in the API:

• DBAL\Driver\Connection::beginTransaction() returns bool
• its implementation DBAL\Connection::beginTransaction() returns void!

[guilhermeblanco]

@BenMorel Why not:

• Create Connection::createTransaction() to return DBAL\Transaction through a TransactionManager
• Create EntityManager::createTransaction() to return ORM\Transaction
  This differs from DBAL\Transaction since we may control this through X/Open XA or some sort of DRDA (Distributed Relational Database Architecture). This falls into what I mentioned earlier that data can come from multiple data sources.
• Deprecate Connection::beginTransaction() and EntityManager::beginTransaction()
• Deprecate Connection::commit() and EntityManager::commit() in favour of Transaction::commit()
  This would internally call getCurrentTransaction()->commit()
• Deprecate Connection::rollback() and EntityManager::rollback() in favour of Transaction::rollback()
  This would internally call getCurrentTransaction()->rollback()

We keep BC and we also add our own support.
The problems I see is how we store data inside of Transaction. Changesets or scheduled operations?

[BenMorel]

@guilhermeblanco That's where I get lost :-)

Why storing data inside of Transaction? My idea was to simply encapsulate the current behaviour in a Transaction object: underlying database transaction, savepoints, and "rollback only" flag.

Do I miss something?

Otherwise, I like the idea of adding createTransaction() and deprecating beginTransaction(); but should createTransaction() actually begin the transaction, or should we call createTransaction()->begin()?

[BenMorel]

Ok I've starting playing around with this idea, and here is the result: doctrine/dbal#571

I have no idea if it's what you had in mind guys, it can be miles away, but hopefully it's close.

I personally like it, it really de-clutters the Connection and shifts the responsibility of handling transactions to the TransactionManager.

And if we were going that route, I could add a configuration to the Transaction object, and this is where we would set the default lock mode as suggested by @beberlei in the present PR:

$transaction = $em->createTransaction();
$transaction->setDefaultLockMode(LockMode::PESSIMISTIC_WRITE);
// ...
$transaction->commit();

The EntityManager would be left unaffected by the default lock mode after the transaction is closed.

Please let me know what you think by commenting on doctrine/dbal#571!

Note: I didn't add the default lock mode or any kind of configuration yet on the Transaction object, to keep things simple while the draft is being reviewed. This would be the subject of yet another PR on the DBAL.

[mvrhov]

Can we also have the events after the transaction is really commited.
I have a use case here where I issue some DQL queries + some native queries + some changes in EM. All this is wrapped inside begin/commit, the problem is that post flush event happens to early because of that "external transaction" and thus the things that should happen after everything is committed happen to early and fail.

[BenMorel]

@mvrhov You should open a separate issue for this!

[BenMorel]

Ok, I think we're getting somewhere with the transaction object in doctrine/dbal#634:

$manager = $connection->getTransactionManager();
$transaction = $manager->createTransaction() // returns a TransactionBuilder
    ->withIsolationLevel(Connection::TRANSACTION_SERIALIZABLE)
    ->begin();

My only issue now is: how do we add configuration options specific to the ORM?

The TransactionBuilder and TransactionDefinition are part of the DBAL. We could extend them, but that means extending the TransactionManager to return a ORM\TransactionBuilder, that would create a ORM\TransactionDefinition, ... not really clean if you ask me.

I've thought about a generic API for setting any variable:

$transaction = $manager->createTransaction()
    ->with(Connection::ISOLATION_LEVEL, Connection::TRANSACTION_SERIALIZABLE)
    ->with(EntityManager::DEFAULT_LOCK_MODE, LockMode::PESSIMISTIC_WRITE)
    ->begin();

This way, we don't have to extend any classes in the ORM. We just have to define configuration constants in the EntityManager for ORM-specific configurations, just like we would define configuration constants in the Connection for DBAL-specific configurations.

The DBAL would get the isolation level of the transaction with:

$transaction->getTransactionDefinition()->get(Connection::ISOLATION_LEVEL);

The ORM would get the default lock mode of the transaction with:

$transaction->getTransactionDefinition()->get(EntityManager::DEFAULT_LOCK_MODE);

What do you think?

[Ocramius]

@BenMorel getting back to this: does this require evaluation of doctrine/dbal#634 first?

[BenMorel]

@Ocramius Yes, we would need doctrine/dbal#634 merged first!

[BenMorel]

@Ocramius Is there anything preventing this PR (and doctrine/dbal#634) from being merged now? It's been 18 months already, and that would be a nice new feature for Doctrine 2.6.

[DHager]

@BenMorel Sort of out of left-field here, but I'm working on #1456 which might end up involving a new mode LockMode::OPTIMISTIC_FORCE_INCREMENT. While there's nothing intrinsically illegal about someone using it on every entity they load, it would become a very interesting way for a user to shoot themselves in the foot by causing lots of SQL-update activity.

[BenMorel]

@DHager Worst case scenario, we could prevent setting the default lock mode to OPTIMISTIC_FORCE_INCREMENT?

[DHager]

@BenMorel Most likely. I don't think it's a big deal since it won't matter until unless that PR gets in anyway. I mainly wanted to bring it up as a potential edge-case where a locking scheme could backfire if overused.

[BenMorel]

Guys, it's been 2 years since I opened this pull request, but nobody seems to care, neither on this PR, nor on doctrine/dbal#634 it depends on. I'm not far from abandoning them due to the lack of feedback. It's a pity, as it is a feature I needed and still need.

[BenMorel]

Up. 😑

[BenMorel]

Bump 😢

[DHager]

I know the feeling.

[stof]

Well, this PR depend on a DBAL PR which is not merged, so it has no way to be merged

[BenMorel]

Aren't the maintainers of ORM and DBAL the same people ?

[BenMorel]

Are you still interested in merging this feature, should I rebase it on 3.0?

[Majkl578]

If the Transaction object lands in DBAL, i think some similar approach around it here would be better than global switches.

[BenMorel]

@Majkl578 That's exactly why I'm trying to push the Transaction object into DBAL first 👍

Basically calling createTransaction() on the EntityManager would create an underlying DBAL transaction, but would decorate it with extra configuration such as the default lock mode, that would only be effective for the duration of the transaction.

[Majkl578]

We could probably think about some high-level transaction abstraction in ORM (more like business transactions) and completely shadow the DBAL in there. Just a thought though.

[BenMorel]

That could be an idea. We don't actually need to expose the DBAL transaction at all, we can encapsulate it privately.

[Majkl578]

Exactly, and locking could sit just on top of all that. JPA defines things like EntityTransaction or resource-local EMs or JTA EMs. That may be an interesting approach, if architecturally possible.

[BenMorel]

Let's focus on the Transaction object then, and I'll make a proposal of an ORM transaction here :)