Commons BeanUtils
  1. Commons BeanUtils
  2. BEANUTILS-49

Lock in BeanUtilsBean.getInstance(BeanUtilsBean.java:78)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.0
    • Fix Version/s: 1.8.0
    • Component/s: Bean / Property Utils
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      Commons Beanutils 1.7 introduced a new problem:
      During high traffic times threads begin to wait at the synchronized method
      org.apache.commons.beanutils.BeanUtilsBean.getInstance() which causes the
      complete thread pool to be used up in our Struts 1.2.7 based web application. In
      our live environment this caused all 70 threads to be blocked by the same lock.

      This behaviour can be easily reproduced by a calling a test JSP provided below
      with multiple threads. Using the tool siege to concurrently call the JSP with
      two threads is enough to reproduce the lock scenario, the situation gets worse
      the more concurrent threads are used (i.e. the throughput decreases).

      <!-- begin of test JSP -->
      <%@ taglib uri="/struts-logic.tld" prefix="logic" %>
      <%@ taglib uri="/struts-bean.tld" prefix="bean" %>
      <%@ taglib uri="/struts-tiles.tld" prefix="tiles" %>
      <%@ taglib uri="/struts-html.tld" prefix="html" %>
      <%@ taglib uri="/JLog.tld" prefix="jlog" %>

      <%@ page import="java.util.*"%>
      <%
      final long t0 = System.currentTimeMillis();
      Collection col = new ArrayList();
      for(int i = 0; i<5; i++)

      { org.apache.struts.util.LabelValueBean lvb = new org.apache.struts.util.LabelValueBean("col"+i, "col"+i); col.add(lvb); }

      pageContext.setAttribute("col", col);
      %>
      <logic:notEmpty name="col"><logic:iterate name="col" id="test"><logic:iterate
      name="col" id="test2"><logic:iterate name="col" id="test3"><logic:iterate
      name="col" id="test3"><logic:iterate name="col" id="test4"><logic:iterate
      name="col" id="test4">
      <bean:define id="foo" name="test4" property="value"/><bean:define id="bar"
      name="test4" property="label"/>
      </logic:iterate></logic:iterate></logic:iterate></logic:iterate></logic:iterate></logic:iterate></logic:notEmpty>
      Finished!
      <!-- end of test JSP -->

      A test run with Struts 1.2.7 and Commons Beanutls 1.7.0 reproduces the lock (in
      this example with 10 concurrent threads):
      siege -c10 -r20 "localhost:1701/dcw/test.jsp"
      => Thread dump is full with threads like this:
      "ExecuteThread: '4' for queue: 'weblogic.kernel.Default'" daemon prio=1
      tid=0x083859f8 nid=0x76f4 waiting for monitor entry [7628c000..7628c8bc]
      at
      org.apache.commons.beanutils.BeanUtilsBean.getInstance(BeanUtilsBean.java:78)

      • waiting to lock <0x6c86eab0> (a java.lang.Class)
        at
        org.apache.commons.beanutils.PropertyUtilsBean.getInstance(PropertyUtilsBean.java:101)
        at
        org.apache.commons.beanutils.PropertyUtils.getProperty(PropertyUtils.java:290)
        at org.apache.struts.taglib.TagUtils.lookup(TagUtils.java:950)
        at org.apache.struts.taglib.bean.DefineTag.doEndTag(DefineTag.java:230)
        at jsp_servlet._test._jspService(_test.java:309)
        ...

      This behaviour is new to version 1.7. The same test using Commons Beanutils
      1.6.1 showed no problems: By falling back to the old version we could
      temporarily solve our live problems and could continue to use Struts 1.2.7.

      Nevertheless this should be fixed.

      1. BEANUTILS-49.patch
        6 kB
        Henri Yandell
      2. beanutils-49-ContextClassLoaderLocale.patch
        6 kB
        Niall Pemberton

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        602d 14h 1 Niall Pemberton 06/Jul/07 09:38
        Resolved Resolved Closed Closed
        262d 23h 29m 1 Henri Yandell 25/Mar/08 08:07
        Hide
        Niall Pemberton added a comment -

        I don't currently have any plans to fix this.

        Show
        Niall Pemberton added a comment - I don't currently have any plans to fix this.
        Hide
        Raj Vadheraju added a comment -

        Niall,
        We are facing the issue with get() method being synchronized in ContenxtClassLoaderLocal. I see your patch and it never seem to have applied to BeanUtils where as other patch is applied in 1.8 which helped us greatly.
        Currently, even after taking 1.8, We are seeing several threads waiting on ContenxtClassLoaderLocal.get().
        Do you have plans to apply this fix in next release? Or can you make code change and send it my way?

        Show
        Raj Vadheraju added a comment - Niall, We are facing the issue with get() method being synchronized in ContenxtClassLoaderLocal. I see your patch and it never seem to have applied to BeanUtils where as other patch is applied in 1.8 which helped us greatly. Currently, even after taking 1.8, We are seeing several threads waiting on ContenxtClassLoaderLocal.get(). Do you have plans to apply this fix in next release? Or can you make code change and send it my way?
        Henri Yandell made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Niall Pemberton made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Hide
        Niall Pemberton added a comment -

        Where are we at with this? I agree with you improvments without numbers is not good and like you am not 100% happy with the patch. IMO we should just leave it for now at removing the uncessary synchronization that Kenneth Xu suggested (already committed). We could punt for review again post 1.8.0.

        Show
        Niall Pemberton added a comment - Where are we at with this? I agree with you improvments without numbers is not good and like you am not 100% happy with the patch. IMO we should just leave it for now at removing the uncessary synchronization that Kenneth Xu suggested (already committed). We could punt for review again post 1.8.0.
        Hide
        Henri Yandell added a comment -

        Gut thoughts.....

        • We're improving the performance of something without having any numbers. So a bit iffy there.
        • The code has calls to WeakHashMap.isEmpty with a comment that seems to indicate that this is going to cause the weak references to be re-evaluated. I don't see how we can infer that - in fact the javadoc for WeakHashMap would seem to imply that isEmpty does not re-evaluate things. I got into looking at this out of worry that not synchronizing this 'purge' would cause bugs.
        • While we're here.... "} catch (SecurityException e) { /* SWALLOW - should we log this? */ }

          ". Should we? Should we throw an Exception? [yeah yeah, I know... change of API].

        • Is the unset(ClassLoader) method meant to be public? Is the set(ClassLoader,Object) meant to be private? The class looks designed for extension, should things be protected?
        Show
        Henri Yandell added a comment - Gut thoughts..... We're improving the performance of something without having any numbers. So a bit iffy there. The code has calls to WeakHashMap.isEmpty with a comment that seems to indicate that this is going to cause the weak references to be re-evaluated. I don't see how we can infer that - in fact the javadoc for WeakHashMap would seem to imply that isEmpty does not re-evaluate things. I got into looking at this out of worry that not synchronizing this 'purge' would cause bugs. While we're here.... "} catch (SecurityException e) { /* SWALLOW - should we log this? */ } ". Should we? Should we throw an Exception? [yeah yeah, I know... change of API] . Is the unset(ClassLoader) method meant to be public? Is the set(ClassLoader,Object) meant to be private? The class looks designed for extension, should things be protected?
        Henri Yandell made changes -
        Attachment BEANUTILS-49.patch [ 12346391 ]
        Hide
        Henri Yandell added a comment -

        Improved version of the patch with the return statement back in, and a now incorrect comment removed.

        Show
        Henri Yandell added a comment - Improved version of the patch with the return statement back in, and a now incorrect comment removed.
        Hide
        Henri Yandell added a comment -

        Otherwise, it seems fine to me. +1.

        Show
        Henri Yandell added a comment - Otherwise, it seems fine to me. +1.
        Hide
        Henri Yandell added a comment -

        The patch changes:

        valueByClassLoader.put(contextClassLoader, value);
        return;

        to:

        set(contextClassLoader, value);

        By losing the return statement, the following executes now:

        globalValue = value;
        globalValueInitialized = true;

        So looks like the global value will always be set. Looks like the return; needs to stay.

        Show
        Henri Yandell added a comment - The patch changes: valueByClassLoader.put(contextClassLoader, value); return; to: set(contextClassLoader, value); By losing the return statement, the following executes now: globalValue = value; globalValueInitialized = true; So looks like the global value will always be set. Looks like the return; needs to stay.
        Niall Pemberton made changes -
        Hide
        Niall Pemberton added a comment -

        Attaching a patch with a possible solution. It sacrifices the performance of updating the ContextClassLoaderLocale instance by synchronizing the operations which modify the internal Map and creating a new copy each time anything is added or removed. Since I don't expect there will be many ClassLoader instances it shouldn't be a big penalty - however it means that the vastly higher number of read operations are unsynchronized, improving performance.

        Comments on this would be very welcome

        Show
        Niall Pemberton added a comment - Attaching a patch with a possible solution. It sacrifices the performance of updating the ContextClassLoaderLocale instance by synchronizing the operations which modify the internal Map and creating a new copy each time anything is added or removed. Since I don't expect there will be many ClassLoader instances it shouldn't be a big penalty - however it means that the vastly higher number of read operations are unsynchronized, improving performance. Comments on this would be very welcome
        Niall Pemberton made changes -
        Assignee Niall Pemberton [ niallp ]
        Summary [beanutils] Lock in BeanUtilsBean.getInstance(BeanUtilsBean.java:78) Lock in BeanUtilsBean.getInstance(BeanUtilsBean.java:78)
        Affects Version/s 1.7.0 [ 12311966 ]
        Fix Version/s 1.8.0 [ 12311949 ]
        Bugzilla Id 37441
        Hide
        Niall Pemberton added a comment -

        I agree with Kenneth Xu - the synchronization on the BeanUtilsBean's getInstance()/setInstance() methods are uncessary since all the ContextClassLoaderLocale's methods are already synchronized - so I've removed the synchronization from those methods (and also for LocaleBeanUtilsBean which used the same mechanism).

        http://svn.apache.org/viewvc?view=rev&revision=473972

        Show
        Niall Pemberton added a comment - I agree with Kenneth Xu - the synchronization on the BeanUtilsBean's getInstance()/setInstance() methods are uncessary since all the ContextClassLoaderLocale's methods are already synchronized - so I've removed the synchronization from those methods (and also for LocaleBeanUtilsBean which used the same mechanism). http://svn.apache.org/viewvc?view=rev&revision=473972
        Henri Yandell made changes -
        Assignee Niall Pemberton [ niallp ]
        Hide
        Kenneth Xu added a comment -

        There is no doubt that Struts code should be changed to make best use of beanutils. But I believe there is still some space for beanutils to improve too.

        I think the getInstance is unnecessarily synchronized as the beansByClassLoader.get is already synchronized. Futher, synchronize the entire beansByClassLoader.get method seems to be overkill. As there is so much existing code written to use the static methods in BeanUtils class, it's very likely that the getInstance becomes hotspot in an heavy loaded application. So developers would love to see every bit of performance gain in the getInstance call.

        Show
        Kenneth Xu added a comment - There is no doubt that Struts code should be changed to make best use of beanutils. But I believe there is still some space for beanutils to improve too. I think the getInstance is unnecessarily synchronized as the beansByClassLoader.get is already synchronized. Futher, synchronize the entire beansByClassLoader.get method seems to be overkill. As there is so much existing code written to use the static methods in BeanUtils class, it's very likely that the getInstance becomes hotspot in an heavy loaded application. So developers would love to see every bit of performance gain in the getInstance call.
        Hide
        Niall Pemberton added a comment -

        I agree with Joe's comments, but we should review it here first to see if we can find a solution. I'll try and find time to look at it

        Show
        Niall Pemberton added a comment - I agree with Joe's comments, but we should review it here first to see if we can find a solution. I'll try and find time to look at it
        Henri Yandell made changes -
        Assignee Niall Pemberton [ niallp ]
        Hide
        Henri Yandell added a comment -

        Niall - any opinion? Should we wontfix this and send it Struts-wards to be fixed there?

        Show
        Henri Yandell added a comment - Niall - any opinion? Should we wontfix this and send it Struts-wards to be fixed there?
        Niall Pemberton made changes -
        Bugzilla Id 37441
        Component/s Bean / Property Utils [ 12311458 ]
        Henri Yandell made changes -
        Affects Version/s unspecified [ 12311647 ]
        Project Commons [ 12310458 ] Commons BeanUtils [ 12310460 ]
        Assignee Jakarta Commons Developers Mailing List [ commons-dev@jakarta.apache.org ]
        Key COM-2553 BEANUTILS-49
        Component/s BeanUtils [ 12311101 ]
        Henri Yandell made changes -
        Field Original Value New Value
        issue.field.bugzillaimportkey 37441 12342705
        Hide
        Joe Germuska added a comment -

        That this causes locking problems is probably not a bug in commons-beanutils, or
        even if it is (in the sense of impact to previously available features), it's
        probably not something which should be reverted.

        The refactoring to support a BeanUtilsBean instead of just having everything be
        static in the utils class is a valuable one, because it allows people to have
        different conversion rules in different cases, rather than being bound to one
        static set of conversion rules in a single class loader.

        In the specific context where you raised this issue, I think we should look at
        changing Struts to use BeanUtilsBean instances instead of static calls to
        getInstance. This may be very easy, or it may turn out to be kind of tricky.

        I'm not going to change the assignment or severity of this ticket just yet, but
        right now in my mind this is more an issue about how Struts uses
        commons-beanutils and not a bug in commons-beanutils.

        any other opinions?

        Show
        Joe Germuska added a comment - That this causes locking problems is probably not a bug in commons-beanutils, or even if it is (in the sense of impact to previously available features), it's probably not something which should be reverted. The refactoring to support a BeanUtilsBean instead of just having everything be static in the utils class is a valuable one, because it allows people to have different conversion rules in different cases, rather than being bound to one static set of conversion rules in a single class loader. In the specific context where you raised this issue, I think we should look at changing Struts to use BeanUtilsBean instances instead of static calls to getInstance. This may be very easy, or it may turn out to be kind of tricky. I'm not going to change the assignment or severity of this ticket just yet, but right now in my mind this is more an issue about how Struts uses commons-beanutils and not a bug in commons-beanutils. any other opinions?
        Jesper Richter-Reichhelm created issue -

          People

          • Assignee:
            Niall Pemberton
            Reporter:
            Jesper Richter-Reichhelm
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development