OpenJPA
  1. OpenJPA
  2. OPENJPA-437

EntityManagerFactory is not thread-safe

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0, 1.0.1, 1.0.2, 1.1.0
    • Fix Version/s: 1.0.2, 1.1.0
    • Component/s: kernel
    • Labels:
      None

      Description

      Under certain conditions, we have discovered that the EntityManagerFactory is not thread safe when creating EntityManagers. The problem is in the loadPersistentTypes method of the AbstractBrokerFactory. There is an unprotected data structure (_pcClassNames) that can various problems (NullPointerException, IndexOutOfBoundsException, etc) when attempting to add new elements to the ArrayList. Other similar datastructures in this part are properly synchronized (_pcClassLoaders for example), but somehow we missed this one.

      A common scenario where this might be encountered is if your SLSB has an injected PersistenceUnit (EntityManagerFactory), but is attempting to create the EntityManager during a post-bean creation method (@PostConstruct). In this case, the SLSB instances are probably using the same EMF instance (of course this would depend on your application server implementation). If you have this type of environment, then hitting these SLSB's with multiple clients could get you into this situation.

        Issue Links

          Activity

          Hide
          Patrick Linskey added a comment -

          A possible fix has been checked in for OPENJPA-449.

          Show
          Patrick Linskey added a comment - A possible fix has been checked in for OPENJPA-449 .
          Hide
          Patrick Linskey added a comment -

          Make that "a partial fix". OPENJPA-449 only deals with _pcClassLoaders.

          Show
          Patrick Linskey added a comment - Make that "a partial fix". OPENJPA-449 only deals with _pcClassLoaders.
          Hide
          Kevin Sutter added a comment -

          Looking at just synchronizing the AbstractBrokerFactory.loadPersistentTypes method to resolve this problem. As I continued to look at this problem, both _pcClassNames and _pcClassLoaders are not synchronized. Patrick's fix for OPENJPA-449 to move the creation of the _pcClassLoaders structure to the constructor closed the window for the scenario described in OPENJPA-449, but it's still not protected from multiple client/thread access. (Note, my previous comments in the Description about _pcClassLoaders being protected were not accurate.)

          I could change both of these Collections to be synchronized, but the code was getting a bit ugly due to the still required synchronization for the iterators. Since these two Collections are only modified in this method, an easy solution would be to make the method Synchronized. Since this method is only used when creating new brokers, this shouldn't affect the through put.

          Also, the logic for this method is based on the return value of the loadPersistentTypes on the MetaDataRepository instance. This method is also synchronized, so we'd be following suit.

          Any concerns about going the route of synchronizing the AbstractBrokerFactory.loadPersistentTypes method?

          Kevin

          Show
          Kevin Sutter added a comment - Looking at just synchronizing the AbstractBrokerFactory.loadPersistentTypes method to resolve this problem. As I continued to look at this problem, both _pcClassNames and _pcClassLoaders are not synchronized. Patrick's fix for OPENJPA-449 to move the creation of the _pcClassLoaders structure to the constructor closed the window for the scenario described in OPENJPA-449 , but it's still not protected from multiple client/thread access. (Note, my previous comments in the Description about _pcClassLoaders being protected were not accurate.) I could change both of these Collections to be synchronized, but the code was getting a bit ugly due to the still required synchronization for the iterators. Since these two Collections are only modified in this method, an easy solution would be to make the method Synchronized. Since this method is only used when creating new brokers, this shouldn't affect the through put. Also, the logic for this method is based on the return value of the loadPersistentTypes on the MetaDataRepository instance. This method is also synchronized, so we'd be following suit. Any concerns about going the route of synchronizing the AbstractBrokerFactory.loadPersistentTypes method? Kevin
          Hide
          Kevin Sutter added a comment -

          Resolved in both 1.0.x and 1.1.0 branches (svn 612846 revision).

          Show
          Kevin Sutter added a comment - Resolved in both 1.0.x and 1.1.0 branches (svn 612846 revision).
          Hide
          Patrick Linskey added a comment -

          I'm a bit concerned about adding the synchronization. While this addresses the thread safety issue, it also introduces a bottleneck for applications with a large number of concurrent threads creating new EntityManagers. This was not a bottleneck in the previous (unsafe) code since the synchronized repository call was only made once.

          Attached is a patch that I believe is both thread-safe and unsynchronized. There might be a better datastructure other than a ConcurrentReferenceHashSet, but I think that it should do the job.

          Show
          Patrick Linskey added a comment - I'm a bit concerned about adding the synchronization. While this addresses the thread safety issue, it also introduces a bottleneck for applications with a large number of concurrent threads creating new EntityManagers. This was not a bottleneck in the previous (unsafe) code since the synchronized repository call was only made once. Attached is a patch that I believe is both thread-safe and unsynchronized. There might be a better datastructure other than a ConcurrentReferenceHashSet, but I think that it should do the job.
          Hide
          Patrick Linskey added a comment -

          Reopening pending discussion of synchronization.

          Show
          Patrick Linskey added a comment - Reopening pending discussion of synchronization.
          Hide
          Kevin Sutter added a comment -

          Patrick,
          Thanks for taking a look at this.

          I went through the same type of concerns when I was coming up with the original patch. But, it was my understanding that the ConcurrentReferenceHashSet still has an unprotected Iterator. So, later on in this same loadPersistentTypes() method, the _pcClassNames structure is iterated through. To make this iterator safe, then I would have to synchronize around this as well. Also, the _pcClassLoaders should also be made thread-safe since we're updating that structure in this same method. When I put in all of the necessary safeguards, it looked like it might be cleaner just by making the method synchronized.

          But, if we're hitting a bottleneck by making that method synchronized, then we probably need to change it. I had not discovered that bottleneck yet...

          So, do we need to add the following items to your patch?

          o Initialize _pcClassLoaders with a ConcurrentReferenceHashSet.
          o Protect the _pcClassNames iterator usage with a synch block

          Thanks,
          Kevin

          Show
          Kevin Sutter added a comment - Patrick, Thanks for taking a look at this. I went through the same type of concerns when I was coming up with the original patch. But, it was my understanding that the ConcurrentReferenceHashSet still has an unprotected Iterator. So, later on in this same loadPersistentTypes() method, the _pcClassNames structure is iterated through. To make this iterator safe, then I would have to synchronize around this as well. Also, the _pcClassLoaders should also be made thread-safe since we're updating that structure in this same method. When I put in all of the necessary safeguards, it looked like it might be cleaner just by making the method synchronized. But, if we're hitting a bottleneck by making that method synchronized, then we probably need to change it. I had not discovered that bottleneck yet... So, do we need to add the following items to your patch? o Initialize _pcClassLoaders with a ConcurrentReferenceHashSet. o Protect the _pcClassNames iterator usage with a synch block Thanks, Kevin
          Hide
          Patrick Linskey added a comment -

          Good points. I resolved both with my most recent commit on this issue.

          Show
          Patrick Linskey added a comment - Good points. I resolved both with my most recent commit on this issue.
          Hide
          Patrick Linskey added a comment -

          In an offline discussion with Kevin, I realized that _pcClassNames is now read-only, so I think we should be able to replace it with a regular HashSet instead of a ConcurrentReferenceHashSet. Does that sound correct?

          Show
          Patrick Linskey added a comment - In an offline discussion with Kevin, I realized that _pcClassNames is now read-only, so I think we should be able to replace it with a regular HashSet instead of a ConcurrentReferenceHashSet. Does that sound correct?

            People

            • Assignee:
              Kevin Sutter
              Reporter:
              Kevin Sutter
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development