OpenJPA
  1. OpenJPA
  2. OPENJPA-359

OptimisticLockException NOT thrown for entity using Timestamp Version when update from concurrent persistence contexts

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.0.0
    • Fix Version/s: 1.1.0
    • Component/s: jdbc
    • Labels:
      None
    • Environment:
      WIntel 32

      Description

      We ran a test using Timestamp as the version field in an entity, the following (pseudo) test failed when an OptimisticLockException is expected:

      em1.persist( e0(pk1) );

      e1 = em1.find(pk1);
      e2 = em2.find(pk1);

      e1.setAttr( "new1");
      e2.setAttr( "new2");

      em1.merge( e1 );
      em2.merge( e2 ); <<<< Expect an OptimisticLockException

      The cause of this problem is because the TimestampVersionStrategy.nextVersion returns a java.sql.Timestamp(System.currentTimeMillis()); In the Wintel environment, the currentTimeMillis() only has approximately 15ms resolution. When 2 subsequent Timestamp version objects are requested within this 15ms interval, both has the same version value. Therefore the em2.merge does not detected the versions difference between o1 and o2, hence no exception is thrown.

      Due to this behavior, the same test case may failed intermittenly depends on the currentTimeMillis() resolution and the time when a timestamp version is created. From some preliminary tests, the resolution for wintel, linux and z/os are about 15ms, 2ms and 2ms respectively.

      1. OPENJPA-359.1.patch
        8 kB
        Albert Lee
      2. OPENJPA-359.2.patch
        10 kB
        Albert Lee
      3. OPENJPA-359.patch
        3 kB
        Albert Lee

        Activity

        Hide
        Albert Lee added a comment -

        The solution is to create a java.sql.Timestamp based on the current time plus a unique counter within the same current time windows. Therefore the timestamp is not truely a time based timestamp but a unique timestamp for versioning purpose.

        Show
        Albert Lee added a comment - The solution is to create a java.sql.Timestamp based on the current time plus a unique counter within the same current time windows. Therefore the timestamp is not truely a time based timestamp but a unique timestamp for versioning purpose.
        Hide
        Patrick Linskey added a comment -

        We need to also consider clustered environments – this patch doesn't do much for such environments. Of course, in a clustered environment, timestamp-based checks rely on clock synchronization, which is a hard problem.

        I've always seen this as an insoluble problem with timestamp versioning, and have just worked around it by putting sleeps in test cases that test concurrency and timestamp versioning.

        The proposed change adds a synchronized block, which seems like a potential bottleneck; if we decide that we care about this problem, I think I'd rather see us just put a Thread.currentThread().wait(<15 | 2>) call into the versioning code. Note, of course, that this still won't solve the problem in a clustered environment.

        Show
        Patrick Linskey added a comment - We need to also consider clustered environments – this patch doesn't do much for such environments. Of course, in a clustered environment, timestamp-based checks rely on clock synchronization, which is a hard problem. I've always seen this as an insoluble problem with timestamp versioning, and have just worked around it by putting sleeps in test cases that test concurrency and timestamp versioning. The proposed change adds a synchronized block, which seems like a potential bottleneck; if we decide that we care about this problem, I think I'd rather see us just put a Thread.currentThread().wait(<15 | 2>) call into the versioning code. Note, of course, that this still won't solve the problem in a clustered environment.
        Hide
        Albert Lee added a comment -

        >> We need to also consider clustered environments – this patch doesn't do much for such environments. Of course, in a clustered environment, timestamp-based checks rely on clock synchronization, which is a hard problem.

        When you say clustered environment, do you mean multiple appl servers (in a cluster) accessing the same db server? This patch refines the time stamp granularity and improves version uniqueness. It does not degrade the current implementation and will exhibit the exact behavior as far as "clustering" is concern. I agree with you that Versioning support in cluster environment is a hard problem and it is not only for Timestamp but any type. Until there is a central "server" that hands out unique version id, it will remain to be a insoluble problem.

        >> The proposed change adds a synchronized block, which seems like a potential bottleneck;
        The synchronized block is required to make sure the counter is updated correctly in multi-thread scenario. This follows a similar pattern as implemented in the UUIDGenerator. I have considered the bottleneck condition and scaled down the instructions absolutely needed in the synchronized block to improve concurrency.

        >> I think I'd rather see us just put a Thread.currentThread().wait(<15 | 2>) call into the versioning code.
        Do you mean "Thread.currentThread().sleep(<15|2>)" ?
        First, one has to determine the value to sleep. This value varies a lot and depends on the hardware platform and realistically should be determine at run-time. Second, sleeping for 2ms per se is an artificial performance and concurrency inhibitors which I don't recommend.

        We may be able to figure out other means to avoid the synchronized block but still get the same result.

        >> Note, of course, that this still won't solve the problem in a clustered environment.
        Distributed system synchronization is always a hard problem to solve. The initial time stamp value is based on System.currentTimeMillis(), this means the first problem is to make sure this base value is synchronized between all servers in the cluster. The second problem is the increment value needs to be either synchronized and/or common between all servers.

        If we take a step back and say using Timestamp as version id is inherently problematic (the same argument as using float as primary key) in cluster environment, all we can do is to provide an implementation that can improve the possibility NOT to run into a problem condition, as we already have encountered in our test scenario.

        Albert Lee.

        Show
        Albert Lee added a comment - >> We need to also consider clustered environments – this patch doesn't do much for such environments. Of course, in a clustered environment, timestamp-based checks rely on clock synchronization, which is a hard problem. When you say clustered environment, do you mean multiple appl servers (in a cluster) accessing the same db server? This patch refines the time stamp granularity and improves version uniqueness. It does not degrade the current implementation and will exhibit the exact behavior as far as "clustering" is concern. I agree with you that Versioning support in cluster environment is a hard problem and it is not only for Timestamp but any type. Until there is a central "server" that hands out unique version id, it will remain to be a insoluble problem. >> The proposed change adds a synchronized block, which seems like a potential bottleneck; The synchronized block is required to make sure the counter is updated correctly in multi-thread scenario. This follows a similar pattern as implemented in the UUIDGenerator. I have considered the bottleneck condition and scaled down the instructions absolutely needed in the synchronized block to improve concurrency. >> I think I'd rather see us just put a Thread.currentThread().wait(<15 | 2>) call into the versioning code. Do you mean "Thread.currentThread().sleep(<15|2>)" ? First, one has to determine the value to sleep. This value varies a lot and depends on the hardware platform and realistically should be determine at run-time. Second, sleeping for 2ms per se is an artificial performance and concurrency inhibitors which I don't recommend. We may be able to figure out other means to avoid the synchronized block but still get the same result. >> Note, of course, that this still won't solve the problem in a clustered environment. Distributed system synchronization is always a hard problem to solve. The initial time stamp value is based on System.currentTimeMillis(), this means the first problem is to make sure this base value is synchronized between all servers in the cluster. The second problem is the increment value needs to be either synchronized and/or common between all servers. If we take a step back and say using Timestamp as version id is inherently problematic (the same argument as using float as primary key) in cluster environment, all we can do is to provide an implementation that can improve the possibility NOT to run into a problem condition, as we already have encountered in our test scenario. Albert Lee.
        Hide
        Patrick Linskey added a comment -

        > When you say clustered environment, do you mean multiple appl servers
        > (in a cluster) accessing the same db server?

        Yes.

        > I agree with you that Versioning support in cluster environment is a hard
        > problem and it is not only for Timestamp but any type. Until there is a central
        > "server" that hands out unique version id, it will remain to be a insoluble problem.

        I don't understand. IMO, versioning in a clustered environment is a trivial problem
        when using the monotonically-incrementing number approach. The only "problems"
        arise when using timestamps, and arguably, these aren't problems, but rather just
        limitations imposed by the timing needs of the application.

        FTR, with a monotonically-incrementing version number, the database itself acts
        as the central server.

        > sleeping for 2ms per se is an artificial performance and concurrency inhibitors
        > which I don't recommend.

        By my definitions, sleeping for 2ms does incur a performance cost, but does not
        incur any concurrency problem at all. In fact, doing so avoids the concurrency
        problem introduced by the new synchronized block.

        Show
        Patrick Linskey added a comment - > When you say clustered environment, do you mean multiple appl servers > (in a cluster) accessing the same db server? Yes. > I agree with you that Versioning support in cluster environment is a hard > problem and it is not only for Timestamp but any type. Until there is a central > "server" that hands out unique version id, it will remain to be a insoluble problem. I don't understand. IMO, versioning in a clustered environment is a trivial problem when using the monotonically-incrementing number approach. The only "problems" arise when using timestamps, and arguably, these aren't problems, but rather just limitations imposed by the timing needs of the application. FTR, with a monotonically-incrementing version number, the database itself acts as the central server. > sleeping for 2ms per se is an artificial performance and concurrency inhibitors > which I don't recommend. By my definitions, sleeping for 2ms does incur a performance cost, but does not incur any concurrency problem at all. In fact, doing so avoids the concurrency problem introduced by the new synchronized block.
        Hide
        Albert Lee added a comment -

        > I don't understand. IMO, versioning in a clustered environment is a trivial problem
        when using the monotonically-incrementing number approach. The only "problems"
        arise when using timestamps, and arguably, these aren't problems, but rather just
        limitations imposed by the timing needs of the application.
        >
        > FTR, with a monotonically-incrementing version number, the database itself acts
        as the central server.

        If the version is handed off from the db server, then I agree with you that is is a trivial problem. However in our implementation the version is handed off from an instance of the persistence provider of app server (in a cluster). E.g.

        NumberVersionStrategy.nextVersion()

        { .... return Numbers.valueOf(((Number) version).intValue() + 1); }

        TimestampVersionStrategy.netxtVersion()

        { return TimestampHelper.getCurrentTimestamp(); }

        TImestamp is always problematic, regardless of how accurate the value is used. It also depends on how precise (how many fractional digits) the column in the db are being stored. I ran into a scenario where the Timestamp version is precise to the 100ns but the test still failed. It is because the version column in the db only holds up to ms.

        > By my definitions, sleeping for 2ms does incur a performance cost, but does not
        incur any concurrency problem at all. In fact, doing so avoids the concurrency
        problem introduced by the new synchronized block.

        Sleeping in a thread to avoid duplicate time stamp only solve its own problem. What about if there are 2 threads coming in at the same time (within the 15ms time window) and asking for a new version. Without the synchronized block to hand out the next value, the time stamp version created in both threads will be same if sleep is used, which does not solve the initial problem. Even with the current suggested solution, it does not guarantee to solve the cluster scenario.

        Show
        Albert Lee added a comment - > I don't understand. IMO, versioning in a clustered environment is a trivial problem when using the monotonically-incrementing number approach. The only "problems" arise when using timestamps, and arguably, these aren't problems, but rather just limitations imposed by the timing needs of the application. > > FTR, with a monotonically-incrementing version number, the database itself acts as the central server. If the version is handed off from the db server, then I agree with you that is is a trivial problem. However in our implementation the version is handed off from an instance of the persistence provider of app server (in a cluster). E.g. NumberVersionStrategy.nextVersion() { .... return Numbers.valueOf(((Number) version).intValue() + 1); } TimestampVersionStrategy.netxtVersion() { return TimestampHelper.getCurrentTimestamp(); } TImestamp is always problematic, regardless of how accurate the value is used. It also depends on how precise (how many fractional digits) the column in the db are being stored. I ran into a scenario where the Timestamp version is precise to the 100ns but the test still failed. It is because the version column in the db only holds up to ms. > By my definitions, sleeping for 2ms does incur a performance cost, but does not incur any concurrency problem at all. In fact, doing so avoids the concurrency problem introduced by the new synchronized block. Sleeping in a thread to avoid duplicate time stamp only solve its own problem. What about if there are 2 threads coming in at the same time (within the 15ms time window) and asking for a new version. Without the synchronized block to hand out the next value, the time stamp version created in both threads will be same if sleep is used, which does not solve the initial problem. Even with the current suggested solution, it does not guarantee to solve the cluster scenario.
        Hide
        Patrick Linskey added a comment -

        > If the version is handed off from the db server, then I agree with
        > you that is is a trivial problem. However in our implementation
        > the version is handed off from an instance of the persistence
        > provider of app server (in a cluster). E.g.
        >
        > NumberVersionStrategy.nextVersion()

        { > ... > return Numbers.valueOf(((Number) version).intValue() + 1); > }

        Yes, but then, the previous number is used in an UPDATE or DELETE statement. If the row being updated / deleted has been changed by a different VM, then the database will have a different version value at that time, and the update / delete statement will fail (modified row count of 0). So, while we don't fetch the number from the database, the database always contains the most-recent clean version number, and updates only get into the database when the reader had read that value.

        Timestamps are more complicated, as has been discussed on this thread.

        > Sleeping in a thread to avoid duplicate time stamp only solve its own problem.
        > What about if there are 2 threads coming in at the same time (within the 15ms
        > time window) and asking for a new version. Without the synchronized block to
        > hand out the next value, the time stamp version created in both threads will be
        > same if sleep is used, which does not solve the initial problem. Even with the
        > current suggested solution, it does not guarantee to solve the cluster scenario.

        I agree – this entire domain is rife with problems when transactions are shorter than the resolution available. I just do not think that adding synchronization to attempt to partially fix the situation is really worth the cost in all the use cases where transactions are known to be longer than the resolution.

        In any event, I understand that there is some value to what you're proposing; I just don't think that we should change our current behavior, because it's good enough for many users, and has characteristics (lack of synchronization) that is advantageous over the incremental improvement that you're suggesting. So, I could see room for a new versioning strategy or some sort of option to configure how the timestamp behavior works.

        Show
        Patrick Linskey added a comment - > If the version is handed off from the db server, then I agree with > you that is is a trivial problem. However in our implementation > the version is handed off from an instance of the persistence > provider of app server (in a cluster). E.g. > > NumberVersionStrategy.nextVersion() { > ... > return Numbers.valueOf(((Number) version).intValue() + 1); > } Yes, but then, the previous number is used in an UPDATE or DELETE statement. If the row being updated / deleted has been changed by a different VM, then the database will have a different version value at that time, and the update / delete statement will fail (modified row count of 0). So, while we don't fetch the number from the database, the database always contains the most-recent clean version number, and updates only get into the database when the reader had read that value. Timestamps are more complicated, as has been discussed on this thread. > Sleeping in a thread to avoid duplicate time stamp only solve its own problem. > What about if there are 2 threads coming in at the same time (within the 15ms > time window) and asking for a new version. Without the synchronized block to > hand out the next value, the time stamp version created in both threads will be > same if sleep is used, which does not solve the initial problem. Even with the > current suggested solution, it does not guarantee to solve the cluster scenario. I agree – this entire domain is rife with problems when transactions are shorter than the resolution available. I just do not think that adding synchronization to attempt to partially fix the situation is really worth the cost in all the use cases where transactions are known to be longer than the resolution. In any event, I understand that there is some value to what you're proposing; I just don't think that we should change our current behavior, because it's good enough for many users, and has characteristics (lack of synchronization) that is advantageous over the incremental improvement that you're suggesting. So, I could see room for a new versioning strategy or some sort of option to configure how the timestamp behavior works.
        Hide
        Albert Lee added a comment -

        Patrick,

        I understand your concern of the proposed changed.

        I'll continue to find other implementation that does not require a synchronized block plus improving granularity of the version value (time-wise).

        Thanks.

        Show
        Albert Lee added a comment - Patrick, I understand your concern of the proposed changed. I'll continue to find other implementation that does not require a synchronized block plus improving granularity of the version value (time-wise). Thanks.
        Hide
        Patrick Linskey added a comment -

        One approach might be to just create an implementation that uses System.nanoTime(). This would only work in 1.5 environments, but that might be good enough.

        An easy way to do this would be to make a new abstract superclass of TimestampVersionStrategy, and two implementations for providing a timestamp: one that uses the nano calls, and one that uses the milli calls. For extra credit, you could even make the Configuration framework choose which to use by default based on the value of JavaVersions.VERSION.

        Show
        Patrick Linskey added a comment - One approach might be to just create an implementation that uses System.nanoTime(). This would only work in 1.5 environments, but that might be good enough. An easy way to do this would be to make a new abstract superclass of TimestampVersionStrategy, and two implementations for providing a timestamp: one that uses the nano calls, and one that uses the milli calls. For extra credit, you could even make the Configuration framework choose which to use by default based on the value of JavaVersions.VERSION.
        Hide
        Albert Lee added a comment -

        Attached is an alternative implementation of the nano precision timestamp versioning. This is based on the JRE 1.5 System.nanoTime() support. It does not use synchronization block and should be thread-safe.

        A new NanoPrecisionTimestampVersionStrategy is created using alias "nano-timestamp". This is the default date/timestamp version strategy if Java version >= 5.

        Due to the JRE 1.4 compilation requirement in the maven build process, the new TimestampHelper class in the openjpa-persistence module uses Reflection to invoke System.nanoTime() and TimeStamp.setNanos() methods. If there is other alternative to get around the 1.4 maven compilation problem, I would prefer to call these methods directly but I don't have a good solution other than using Reflection. Suggestion is welcome.

        I'll wait until EOD Monday to commit this change.

        Thanks,
        Albert Lee.

        Show
        Albert Lee added a comment - Attached is an alternative implementation of the nano precision timestamp versioning. This is based on the JRE 1.5 System.nanoTime() support. It does not use synchronization block and should be thread-safe. A new NanoPrecisionTimestampVersionStrategy is created using alias "nano-timestamp". This is the default date/timestamp version strategy if Java version >= 5. Due to the JRE 1.4 compilation requirement in the maven build process, the new TimestampHelper class in the openjpa-persistence module uses Reflection to invoke System.nanoTime() and TimeStamp.setNanos() methods. If there is other alternative to get around the 1.4 maven compilation problem, I would prefer to call these methods directly but I don't have a good solution other than using Reflection. Suggestion is welcome. I'll wait until EOD Monday to commit this change. Thanks, Albert Lee.
        Hide
        Albert Lee added a comment -

        Attached again and grant ASF license.

        Show
        Albert Lee added a comment - Attached again and grant ASF license.
        Hide
        Patrick Linskey added a comment -

        We could get rid of at least some of the reflection, and push any remaining reflection to deploy-time only, by moving the new class to the openjpa-jdbc-5 module.

        Show
        Patrick Linskey added a comment - We could get rid of at least some of the reflection, and push any remaining reflection to deploy-time only, by moving the new class to the openjpa-jdbc-5 module.
        Hide
        Albert Lee added a comment -

        Patrick,

        Thanks for the hint... I am able to get around the 1.4 compile and Reflection "problems" by new'ing a single instance of TimestampHelper with an override to Timestamp5elper at runtime if run in Java 5.

        Please review the current solution.

        Albert Lee.

        Show
        Albert Lee added a comment - Patrick, Thanks for the hint... I am able to get around the 1.4 compile and Reflection "problems" by new'ing a single instance of TimestampHelper with an override to Timestamp5elper at runtime if run in Java 5. Please review the current solution. Albert Lee.

          People

          • Assignee:
            Albert Lee
            Reporter:
            Albert Lee
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development