MyFaces Core
  1. MyFaces Core
  2. MYFACES-3442

Infinite loop when calling ApplicationImpl._handleListenerForAnnotations under heavy load

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.3
    • Fix Version/s: 2.0.12, 2.1.6
    • Component/s: None
    • Labels:
      None

      Description

      Hi,

      when doing Load Tests, we recognize that we sometimes got stuck in an infinite loop, when calling the _handleListenerForAnnotations method of the ApplicationImpl class. According to our analysis, this is related to the _classToListenerForMap attribute that is is defined as plain HashMap, so no synchronisation takes place when it is accessed from several threads. Our suggestion would be, to define it as a ConcurrentHashMap instead, because according to our experience, such kind of infinite loops due to concurrency don't occur with ConcurrentHashMaps.

      Kind regards,

      Michael

        Activity

        Michael Dietrich created issue -
        Hide
        Leonardo Uribe added a comment -

        I have changed the affected locations with ConcurrentHashMap and CopyOnWriteArrayList. Thanks to Michael Dietrich for the suggestion.

        Show
        Leonardo Uribe added a comment - I have changed the affected locations with ConcurrentHashMap and CopyOnWriteArrayList. Thanks to Michael Dietrich for the suggestion.
        Leonardo Uribe made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Leonardo Uribe [ lu4242 ]
        Fix Version/s 2.0.12 [ 12319175 ]
        Fix Version/s 2.1.6 [ 12319173 ]
        Resolution Fixed [ 1 ]
        Hide
        Leonardo Uribe added a comment -

        It seems ConcurrentHashMap cannot be used because it is required to support null values. So I changed the solution to use a synchronized block over the map. That should do the trick.

        Show
        Leonardo Uribe added a comment - It seems ConcurrentHashMap cannot be used because it is required to support null values. So I changed the solution to use a synchronized block over the map. That should do the trick.
        Leonardo Uribe made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Leonardo Uribe made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Michael Dietrich added a comment -

        Hi Leonardo,

        at first let me thank you for you fast response and support.

        We also saw that the Unit Tests were failing, when we changed the HashMap to a ConcurrentHashMap, due to the null values you try to store, to indiciate, that no annotations were found. Synchronizing the complete block is surely a solution to the issue, but if this method is called quite often, it might get a bottleneck, because many session might be ligned up waiting to enter the method. Do you have an idea how often the method is normally called? To get around this synchronisation issue, we were thinking to use a Singleton object and put this into the Map, to indicate not annotations have been found. This surely means more changes, because every check for the null retun value nust be changed to check for this Singleton, but in regards to performance it might be the better solution.

        Kind regards,

        Michael

        Show
        Michael Dietrich added a comment - Hi Leonardo, at first let me thank you for you fast response and support. We also saw that the Unit Tests were failing, when we changed the HashMap to a ConcurrentHashMap, due to the null values you try to store, to indiciate, that no annotations were found. Synchronizing the complete block is surely a solution to the issue, but if this method is called quite often, it might get a bottleneck, because many session might be ligned up waiting to enter the method. Do you have an idea how often the method is normally called? To get around this synchronisation issue, we were thinking to use a Singleton object and put this into the Map, to indicate not annotations have been found. This surely means more changes, because every check for the null retun value nust be changed to check for this Singleton, but in regards to performance it might be the better solution. Kind regards, Michael
        Hide
        Leonardo Uribe added a comment -

        Hi Michael

        Really it should be similar to javax.faces.component._ComponentAttributesMap.getPropertyDescriptor() . Note the synchronized block is only to wrap the code that adds a value on the map, and the map is application scope, so once the first requests has been processed, the map will be full and no contention will occur anymore.

        Anyway, it sounds good to use a Singleton object like a empty list (Collections.emptyList()). I have already committed the solution. I hope this change will solve the problem. Thanks for the suggestion.

        Show
        Leonardo Uribe added a comment - Hi Michael Really it should be similar to javax.faces.component._ComponentAttributesMap.getPropertyDescriptor() . Note the synchronized block is only to wrap the code that adds a value on the map, and the map is application scope, so once the first requests has been processed, the map will be full and no contention will occur anymore. Anyway, it sounds good to use a Singleton object like a empty list (Collections.emptyList()). I have already committed the solution. I hope this change will solve the problem. Thanks for the suggestion.
        Hide
        Michael Dietrich added a comment -

        Hi Leonardo,

        sorry for reopening the issue again, but I just downloaded 2.1.6 via SVN and the changes are not contained in there. I can only find them, if I download "current21". According to the Fix Version/s, it should be contained in 2.0.12 and 2.1.6. Is this a mistake and should it be 2.1.7, or can you downport the chages to 2.1.6?

        Thanks and kind regards,

        Michael

        Show
        Michael Dietrich added a comment - Hi Leonardo, sorry for reopening the issue again, but I just downloaded 2.1.6 via SVN and the changes are not contained in there. I can only find them, if I download "current21". According to the Fix Version/s, it should be contained in 2.0.12 and 2.1.6. Is this a mistake and should it be 2.1.7, or can you downport the chages to 2.1.6? Thanks and kind regards, Michael
        Michael Dietrich made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Leonardo Uribe added a comment -

        Hi Michael

        The branch on the svn will be rollback this week (it belongs to a release procedure done in december that does not passed), so you have to check trunk. or 2.0.x branch. A release of 2.0.12/2.1.6 will take some time while I include some cool performance enhancements, so stay tuned.

        best regards,

        Leonardo

        Show
        Leonardo Uribe added a comment - Hi Michael The branch on the svn will be rollback this week (it belongs to a release procedure done in december that does not passed), so you have to check trunk. or 2.0.x branch. A release of 2.0.12/2.1.6 will take some time while I include some cool performance enhancements, so stay tuned. best regards, Leonardo
        Leonardo Uribe made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Leonardo Uribe made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Krashan Brahmanjara added a comment -

        Please apply this patch also for 1.1 and 1.2 branch.

        Show
        Krashan Brahmanjara added a comment - Please apply this patch also for 1.1 and 1.2 branch.

          People

          • Assignee:
            Leonardo Uribe
            Reporter:
            Michael Dietrich
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development