Quarkus: LockModeType support with Panache

[loicmathieu]

Description
Panache in both mode (Entity and Repository) don't have primary support for JPA LockModeType. So if we want to implements a lock mechanism we must inject the EntityManager and use the lock method on the entity.
This is not very convenient and moreover there is no way to make a lock without first retrieving the entity so hitting the database 2 times.

Implementation ideas
I have a prototype in progress to add the following methods ;

• PanageEntityBase::lock(LockModeType)
• PanageEntityBase::findById(Id, LockModeType)
• PanageRepositoryBase::lock(Entity, LockModeType)
• PanageRepositoryBase::findById(Id, LockModeType)

I can share my work in progress in a PR, but I still have some issue with the findById methods, they generate a core dump at runtime (still need to understand the code generation code based on ASM).

[FroMage]

I don't know the first thing about these methods, so I'll let @emmanuelbernard @Sanne and @gsmet pitch in on whether that's useful enough to add here.

If they say it is, the proposed API looks right to me.

You get a core dump, so I assume this is in native mode, right?

[loicmathieu]

Basically it's generating SELECT ... FOR UPDATE query via the JPA support.

[emmanuelbernard]

Hey @loicmathieu I reluctant to add them to the panache contract as I think we are falling into the cases you do less than 20% of the time. Injecting the EntityManager seems an acceptable approach to me.

Stef can chime in but I think if we go an port EM methods in the PanacheEntityBase, that would dillute panache value.

[loicmathieu]

Lock is a common operation on database.

If we didn't give first class support, the user needs to play with the EntityManager but also don't have any way to generate a single SELECT ... FOR UPDATE query as it will only be able to use the EntityManager to lock the object after retrieving.

So it's also a performance issue :

//current situation
Enity entity = panacheRepository.findById(1);//generate a select query
entityManager.lock(entity);//generate a (second) select for update query

//proposed change
Enity entity = panacheRepository.findById(1, LockModeType.PESSIMISTIC_WRITE);//generate a (unique) select for update query

I'm agree that it will leak JPA in Panache and it's not a very good design.

Spring Data JPA add a @Lock annotation (that encapsulate a LockModeType so it's coupled with JPA) and also a @QueryHint annotation that make locking possible, maybe we need something similar and not provide the naive implementation I propose (see https://www.baeldung.com/java-jpa-transaction-locks for exemple of the Spring Data lock support).

Anyway, I found Panache already very coupled to JPA with the choice of the name of the method (persist for exemple) and the it works ... when, for exemple, a Mongo repository with Panache will be created I'm not shure it will play well with the exiting methods ...

[FroMage]

What we could do is that I make it possible for people to add magic methods to their own magic entity superclass. You could then write:

public class MyEntityBaseClass extends PanacheEntity {
    @PanacheMagicMethod(MyJpaOperations.class)
    public static <T extends PanacheEntityBase> T findById(Object id, LockModeType lockModeType) {
        throw JpaOperations.implementationInjectionMissing();
    }
}

public class MyJpaOperations extends JpaOperations {
    public static Object findById(Class<?> entityClass, Object id, LockModeType lockModeType) {
        return getEntityManager().find(entityClass, id, lockModeType);
    }
}

This would just wire in any static method annotated with @PanacheMagicMethod and forward the call to the mentioned class with an extra first parameter of type Class<?>. This would let you get that API as if it were upstream. And we can wait to see how many more people want this feature before deciding to add it upstream or not.

Would that work for you?

Note that I wonder if you will end up wanting the same from the queries too, where it would be a bit more complex.

[emmanuelbernard]

On the poerf issue, you can load the entity in the first place from the EntityManager with the find with lock option. You don't have to find by id with panache.

So my main concern is readability creep. If we put lock, we need to duplicate a lot of other Panache methods.
I wonder about the following proposal

Todo.withLock(LockModeType.PESSIMISTIC_WRITE).findById(1);

withLock() return type would return all Panache methods that accept a lock at the EM level (read, queries). I would not sweat too much if it was returning all of panache operations.
Would that be fesible @FroMage ?

[FroMage]

Probably, but I'm not sure it would be less methods to implement. Since we added support for optional parameters such as Sort and query params and Java does not support this except by overloading, we're still going to duplicate all the overloads on the return type of withLock (which must be a new type since those are static methods otherwise).

[emmanuelbernard]

@FroMage my problem is more that when I do Todo.<autocomplete> how many methods do I see?

[FroMage]

Too many, but if we add Todo.withLock you will also see too many. To be fair, they all have the same name, this is due to optional parameters.

[loicmathieu]

I'm playing the devil advocate here but maybe the best is to provide an annotation like Spring data does ... So it will be the work of the extention to generate the correct method call.

With this we will be able to add a lock mechanism to any find* method, we will just need to redefine those methods in the repository interface (or in the Entity).

Or an other solution is to provide some kind of _lock contex_t, possible implementation could look like :

// Proposal 1
WithLock.execute(LockModeType.PESSIMISTIC_WRITE, entity.findById());
WithLock.execute(LockModeType.PESSIMISTIC_WRITE, respository.findById(1));

// Proposal 1
Lock.with(LockModeType.PESSIMISTIC_WRITE);//magic stuff based on context/ThreadLocal ?
entity.findById();
Lock.with(LockModeType.PESSIMISTIC_WRITE);//magic stuff based on context/ThreadLocal ?
respository.findById(1);

[emmanuelbernard]

@FroMage I'll see half of too many if we go the withLock approach. Or the context approach that @loicmathieu mentions.
I don't think the annotation approach would work for Panache as unlike Spring Data we are not imposing usage via interface methods. In panache there is no natural way to host your annotation at the operation level. Not sure I'm super clear.

[loicmathieu]

@emmanuelbernard you are clear enought ;). What I have in mind is that if we want to use this annotation we need to override the inherited findById/find method from the PanacheEntity or the PanacheRepository

@ApplicationScoped
public class ParameterRepository implements PanacheRepositoryBase<Parameter, ParameterId> {
    @Override
    @Lock(LockModeType.PESSIMISTIC_WRITE)
    public Parameter findById(ParameterId parameterId) {
        throw JpaOperations.implementationInjectionMissing();
    }
}

As PanacheRepositoryBase is an interface with default method we cannot call super so it's not very convenient ... at this point maybe this can be reconsider (Quarkus is not yet in version 1 so backward compatibility should not be mandatory).

[emmanuelbernard]

I don't think that's OK to do so because findById with different lock mode for a given entity is a valid use case.

[FroMage]

As PanacheRepositoryBase is an interface with default method we cannot call super so it's not very convenient

You can, it's just that nobody remembers how to do it: PanacheRepositoryBase.super.findById(parameterId)

[loicmathieu]

Thanks @FroMage so there is a way to do it, but it's not very elegant

@ApplicationScoped
public class ParameterRepository implements PanacheRepositoryBase<Parameter, ParameterId> {
    @Lock(LockModeType.PESSIMISTIC_WRITE)
    public Parameter findByIdForUpdate(ParameterId parameterId) {
        return PanacheRepositoryBase.super.findById(parameterId);
    }
}

And I just think that with the current API I can already do this:

@ApplicationScoped
public class ParameterRepository implements PanacheRepositoryBase<Parameter, ParameterId> {
    @Inject EntityManager entityMnager;
    @Lock(LockModeType.PESSIMISTIC_WRITE)
    public Parameter findByIdForUpdate(ParameterId parameterId) {
        Parameter param = findById(parameterId);
        em.lock(param, LockModeType.PESSIMISTIC_WRITE);
        return param;
    }
}

Maybe if we don't have a consensus of a Lock support we can add a Lock section on the Panache Guide with this solution (I can do it with pleasure), this will also work for PanacheEntity I think ...

[emmanuelbernard]

@loicmathieu let's leave with that for now and add a quick message in the doc with a pointer for this issue for people to raise +1 if they want a better solution. Then we can monitor.

[loicmathieu]

Hi
Ok I Will do the documentation update next week and we will see if others have similar needs.

[loicmathieu]

Hello,
I want this to be re-considered as I encounter and issue with the approach advised in the guide.

When you do:

Person person = findById(id);
entityManager.lock(person, LockModeType.PESSIMISTIC_WRITE);

This generates two queries, so it can create a consistency issues. In my use case, I don't load a person by a sequence identifier from a database table, that I then increment and store back in the database. Doing it with this code generates multiple load of the same sequence ID and this is the issue.

Two queries are generated, so the coherence that the lock offers is not guarenteed for multiple threads:

• T1 read the record without lock
• T2 read the record without lock
• T1 read the record and lock it
• (Unknow steps here for when the write occurs)
• IF PESSIMISTIC_WRITE : T2 read the record and when I increment and save it generates and error
• IF PESSIMISTIC_READ: T2 deadlock
• (Unknow steps here for when the write occurs)

So I ended up using directly the entity manager

entityManager.find(Person.class, id, LockModeType.PESSIMISTIC_WRITE);

Using this, I assume multiple threads behave like this:

• T1 read the record and lock it
• T2 wait
• T1 save the record
• T2 read

[Sanne]

@loicmathieu is right, having only a lock method to be applied after the load is not efficient at all.

However let me point out that both APIs are necessary: sometimes you need to upgrade the lock after you already loaded it.

So let's have both methods?

[emmanuelbernard]

What I propose to limit the API complexity is a Book.withLock(PESSIMISTIC).findById(12);
The return type of withLock will hold the methods of book. But since it will be static methods in this case, we need for it to be more hacky than I had hoped.

[loicmathieu]

@emmanuelbernard this sound like a great API and I see how it can be implemented for PanacheRepository but not for PanacheEntity because of the static methods ... static methods really limits the API capability of PanacheEntity (don't shoot at me but maybe this can be revisited before 1.0 : move away from static methods for PanacheEntity).

The implementation could be based on a ThreaLocal that store the lock but I'm not sure thread local plays well in reactive mode ...

Maybe providing the two methods from my original proposal and revisiting the documentation should be sufficient for a start ? I think I still have the implementation somewhere ...

[loicmathieu]

Hi, I found a more easy implementation.
Instead of doing Book.withLock(PESSIMISTIC).findById(12); we can propose a withLock method on PanacheQuery so one could write Book.find("id", 12).withLock(PESSIMISTIC).list();

This will be a little more verbose and will only covers queries with the find() methods and not the shorthand methods list()/stream()/findById() but it's very easy to implement.

With the same mechanics we can also add withHint() if needed.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you!
We are doing this automatically to ensure out-of-date issues does not stay around indefinitely.
If you believe this issue is still relevant please put a comment on it on why and if it truly needs to stay request or add 'pinned' label.

[emmanuelbernard]

I spent some time on this issue last night.
Your solution is interesting @loicmathieu , you don't have lock everywhere but users have a relatively simple workaround by going to the find method.

The big question is whether we are fine with such a forced workaround for users or do we need to offer withLock for all methods including findById.

@FroMage ^

[emmanuelbernard]

BTW I'm not sure withHint is acceptable at least unless one can disable a hint. would withHint("some.hint", null); disable it? @loicmathieu

[emmanuelbernard]

I've been toying with the problem of withLockMode on PanacheEntityBase and like @loicmathieu figured out, the only solution is to use thread local.

My proposal, if we go that way, would be for PanacheEntityBase to have a thread local EntityOperationState that would be set by methods like:

• Book.withLock()
• Book.withHint()
• Book.withFlushMode()

and unset by any of the static methods (or maybe any method both static and non static:

• find*
• list*
• stream*

The drawback is that if someone uses Book.withLock(...); and forget to call find then we ahve a leaked state. But that's possibly acceptable.

@loicmathieu @FroMage

Note that we could start with Loic's approach only with find("id").withLockMode(...) but if we add the more general method later, then it leads to inconsistent ways of going about it (one with withLockMode prefixed, and one with withLockMode suffixed).

[FroMage]

So, first, while I appreciate the origininality of the proposal, I am strongly against adding one static thread-local per entity type, because these simply won't work in reactive settings and the MP-ContextPropagation implementation complexity required to make those work would be horrible.

Second, I think adding an optional param (or 2) for findById and withXXX methods in PanacheQuery is Good Enough™, because those types of queries are not common, as we mentioned earlier, so they don't need to have an equivalent for find/stream variants. People who want to use them will use the PanacheQuery and that's fine.

[loicmathieu]

@emmanuelbernard I'm not very found of ThreadLocal, they have some caveats (possible leaks, compatible with reactive/asynchrone mode?, performance cost).

I think the aproach on PanacheQuery is simple and could be a first good implementation. Personaly I will also add a PanacheEntityBase.findById(Object, LockModeType) method to allow to lock when finding by id as it's a very common construct.

And we could also use PanacheQuery.withLock if you prefere :)

Regarding hints, this can be provided later, but as I set them at the Query layer and not the EntityManayer I don't think it's needed to provides a way to disable them.

[emmanuelbernard]

On Mon 18 Nov 2019 at 10:42, Stéphane Épardaud [e-mail]
wrote:

So, first, while I appreciate the origininality of the proposal, I am
strongly against adding one static thread-local per entity type, because
these simply won't work in reactive settings and the MP-ContextPropagation
implementation complexity required to make those work would be horrible.

Of course they would work. The with* and list* will be used in the same
block of code. Or do you have use cases where you would separate the calls?

[FroMage]

Someone is bound to write code like this at some point:

Book.withLock();
return reactiveCall().thenApply(v -> Book.find("foo"));

It's horrible and wrong, but it could happen. And I am not sure that locks are so common that we need all those thread-locals just for list/stream when the find methods using the query suffice.

If I'm wrong an those methods are really common, then it's something else. But since we started without them, I think they don't fall in the category if common use-cases.

[loicmathieu]

I'm agree with @FroMage if users can break it, it will happen :(

Regarding lock as common or not, the more common usage is can think of is to lock for update a record from the database. That's why I ask for the overload PanacheEntityBase.findById(Object, LockModeType).

[emmanuelbernard]

I hate users, life is so much easier without these constraints ;P
OK, so we go for find only and I think I agree with loic on the overloaded findById(..., LockMode).