Application-layer versus persistence-layer optimistic locking

[Thomas Sødring]

Hi,

I am working on achieving optimistic locking to avoid concurrency / lost
update issues in the core. Basically A and B retrieve the same entity. A
updates the entity, and a few moments later so does B. B however is
missing the update by A.

After discussing this a bit with Petter on #nikita I decided to describe
the problem so that it is documented and to see if anyone has any comments.

To achieve optimistic locking with ETAGs the core uses the following
mechanism. Whenever a single Noark entity (not lists) is returned by an
endpoint, an ETAG is added to the response. An example of this can be
seen in FondsHateoasController.updateFonds()

 return ResponseEntity.status(HttpStatus.CREATED)
                .eTag(updatedFonds.getVersion().toString())
                .body(fondsHateoas);

The outgoing ETAG is generated from a field called version that every
Noark entity has. This field is annotated with @Version in the Entity class.

 @Version
 @Column(name = "version")
 protected Long version;

Hibernate will increase a field annotated with @Version for each update
made to the entity. This field can be used for caching and optimistic
locking. Interestingly repeated updates to an entity that don't actually
change anything, will not result in a updated value. So e.g. setting the
title of a casefile to "Sakstittel" one thousand times will only result
in the version field being updated once. Useful to know for testing
purposes.

Picture the following scenario. Two clients, each retrieve a copy of an
entity. Let's name these copies as A and B. Both clients have the
appropriate ETAG set, in this case it will be set to "0".

Both clients make a change to their copy of the entity and PUT their
changes to the core at the same time. Let's say that As changes are
committed first. The version field is then increased by 1 so the
entity/row in the database now has a version == "1". When B's change is
to be committed, the version fields are not the same and B's change is
rejected as it is based on stale data.existingFonds.setVersion(version);

So what's happening at the moment? The version field is being updated
correctly, but a mismatching version from a PUT request is not resulting
in a exception being thrown. The reason for this is that Hibernate is
too smart. This is the following code to update fonds:

// There is a @Transactional covering this
// version gets its value from the ETAG in the incoming request header
// systemId is the fonds you want to change
public Fonds handleUpdate(String systemId, Long version, Fonds
incomingFonds) {

  // Get the required fonds from the database
  Fonds existingFonds = fondsRepository.findBySystemId(systemId);

  // Here copy all the values you are allowed to copy ....
  existingFonds.setDescription(incomingFonds.getDescription());
  existingFonds.setTitle(incomingFonds.getTitle());

  // Set the version number to our known version
  existingFonds.setVersion(version);

  // send it to the database
  fondsRepository.save(existingFonds);
  return existingFonds;
}

So after A has committed, version=1 and when B goes to commit, we set
the version field to be B's version, "0" so when the commit to the
database occurs, then hibernate should see that the versions don't match
and throw a concurrency exception. It doesn't and the reason is that
hibernate is ignoring the fact that we are updating the version number:

  existingFonds.setVersion(version);

sets the version to 0, but hibernate doesn't care because it knows that
it is actually in possession of a copy of the entity with a version ==
"1" . So when it persists the change to the entity, there is no problem
from hibernates point-of-view.

Now this, I believe, is correct as the JPA standard (that hibernate
implements) says that you cannot change the version field in an update,
it's hibernates job to update the field. Now I am not the person first
to make this assumption as there are plenty of stackoverflow questions
asking this.

A stack overflow question related to this scenario is here:

https://stackoverflow.com/questions/30881071/optimistic-locking-not-throwing-exception-when-manually-setting-version-field

The accepted solution to this problem is to not actually set the
version, when you should have, but instead do the version checking:

public void setVersion(int version) {
  if (this.version != version) {
    throw new YourOwnOptimisticConcurrencyException();
  }
}

Now this works. It's described in the following scenario (T = time)

Scenario 1:
T1. A retrieves entity, ETAG=0
T2. B retrieves entity, ETAG=0
T3. A's PUT request starts, db-version=0, ETAG=0
T4. For A, db-version == 0 && ETAG == 0, request is committed
T5. A's PUT request succeeds, db-version is now == "1"
T6. B's PUT request starts, db-version=1, ETAG=0
T7. A checks db-version = 1 && ETAG == 0, request is rejected

I implemented this yesterday but it is implemented in the service layer
and my fear is that I am handling the concurrency at the wrong layer.
Ideally I want hibernate to throw this exception, not me. I want this to
be detected as close as possible to the persistence layer.

The reason I believe this solution may not solve the issue is the
following scenario:

Scenario 2:

T1. A retrieves entity, ETAG=0
T2. B retrieves entity, ETAG=0
T3. A's PUT request starts, A retrieves the entity with version "0"
T4. B's PUT request starts, B retrieves the entity with version "0"
T5. A checks version == "0", request is committed
T6. A's PUT request succeeds, version is now == "1"
T7. B checks version == "0", request is committed (?)

At T7, B's PUT request should be rejected and it should be hibernate
doing it. Now maybe this is the case but I have not been able to
reproduce the scenario to verify it. The time granularity is too fine to
reproduce it. This is because I need the server to handle the PUT
requests, with db-version=0, at the same time.

However hibernate should know at T7 that the version of the entity for
B's PUT request is not in sync. It *should* then check the version field
and see they are out of sync and throw a StaleObjectStateException The
documentation does seem to suggest that this is the case [1]

[1]
https://docs.jboss.org/hibernate/orm/3.3/reference/en-US/html/transactions.html#transactions-optimistic

Ultimately this is application-layer versus persistence-layer
optimistic locking. At the application-layer, this is not a problem and
the chosen approach is even documented in the hibernate documentation.
But I feel uncomfortable going forward, unable to show that the core can
safely handle Scenario 2.

So if anybody has a experience on to how to test for Scenario 2, or if
they believe the persistence-layer @Version field will kick the required
exception, I'd love to hear it,

As it stands I think we will continue implementing the
application-layer optimistic locking mechanism and assume the
persistence-layer will pick up Scenario 2.

 - Tom