Commons Validator
  1. Commons Validator
  2. VALIDATOR-89

[validator] ValidatorAction needs thread-safe

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3.1 Release
    • Component/s: None
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

      Description

      ValidatorAction needs thread-safe because it is cached by other programs, like
      Struts.
      But ValidatorAction has an unthread-safe block.

      Here is a patch below.

          • ValidatorAction.java Mon Apr 24 22:41:55 2006
          • ValidatorAction.java.new Mon Apr 24 22:44:54 2006
            ***************
          • 527,536 ****
            params.put(Validator.VALIDATOR_ACTION_PARAM, this);

      try {
      ! ClassLoader loader = this.getClassLoader(params);
      ! this.loadValidationClass(loader);
      ! this.loadParameterClasses(loader);
      ! this.loadValidationMethod();

      Object[] paramValues = this.getParameterValues(params);

      — 527,540 ----
      params.put(Validator.VALIDATOR_ACTION_PARAM, this);

      try {
      ! if (this.validationMethod == null) {
      ! synchronized(this)

      { ! ClassLoader loader = this.getClassLoader(params); ! this.loadValidationClass(loader); ! this.loadParameterClasses(loader); ! this.loadValidationMethod(); ! }

      ! }

      Object[] paramValues = this.getParameterValues(params);

        Activity

        Hide
        Henri Yandell added a comment -

        Patch also includes a bit of defensive coding by adding an if(validationMethod == null).

        Given that the methods it calls are all defensive and private, it's not needed. Dunno if it's worth adding -
        doesn't seem like it can hurt.

        Show
        Henri Yandell added a comment - Patch also includes a bit of defensive coding by adding an if(validationMethod == null). Given that the methods it calls are all defensive and private, it's not needed. Dunno if it's worth adding - doesn't seem like it can hurt.
        Hide
        Henri Yandell added a comment -

        Typical POST then DOH moment. Extra defensive coding stops the synchronized block being entered all
        the time.

        Show
        Henri Yandell added a comment - Typical POST then DOH moment. Extra defensive coding stops the synchronized block being entered all the time.
        Hide
        Henri Yandell added a comment -

        Playing at writing a unit test for this - I'm interested in whether this fixes a bug in a previous release or
        just improves the code. Can't have a JUnit class implement Runnable it seems, so bit more code needed
        test-wise.

        Takayuki - does anything bad happen when it's not synchronized? Did you have an exception
        (NullPointerException?), or is this a case of seeing an improvement by looking at the source?

        When I look at the three load method calls, nothing jumps out as likely to break even when running as
        non-threadsafe, but obviously can't have any real idea on that without testing

        Show
        Henri Yandell added a comment - Playing at writing a unit test for this - I'm interested in whether this fixes a bug in a previous release or just improves the code. Can't have a JUnit class implement Runnable it seems, so bit more code needed test-wise. Takayuki - does anything bad happen when it's not synchronized? Did you have an exception (NullPointerException?), or is this a case of seeing an improvement by looking at the source? When I look at the three load method calls, nothing jumps out as likely to break even when running as non-threadsafe, but obviously can't have any real idea on that without testing
        Hide
        Takayuki Kaneko added a comment -

        Henri,

        Yes, I had an exception related this issue. Here is an exception below.

        [2006/04/06 16:00:07.727][ERROR][ValidatorForm 112 line
        ]com.example.validator.FieldChecks.validateId(java.lang.Object, null, null,
        null, null, null)
        org.apache.commons.validator.ValidatorException:
        com.example.validator.FieldChecks.validateId(java.lang.Object, null, null, null,
        null, null)
        at
        org.apache.commons.validator.ValidatorAction.loadValidationMethod(ValidatorAction.java:627)
        at
        org.apache.commons.validator.ValidatorAction.executeValidationMethod(ValidatorAction.java:557)
        at org.apache.commons.validator.Field.validateForRule(Field.java:827)
        at org.apache.commons.validator.Field.validate(Field.java:907)
        at org.apache.commons.validator.Form.validate(Form.java:174)
        at org.apache.commons.validator.Validator.validate(Validator.java:367)
        at org.apache.struts.validator.ValidatorForm.validate(ValidatorForm.java:110)
        at
        org.apache.struts.action.RequestProcessor.processValidate(RequestProcessor.java:928)
        at org.apache.struts.action.RequestProcessor.process(RequestProcessor.java:204)

        I use commons-validator 1.1.4 with Struts 1.2.7.
        This exception wasn't occured when I accessed with browser. It was occured under
        the load testing with JMeter.
        So, I can't attach the unit test code.

        In my opiniton, the three load methods needs synchronization.
        I modified the code with this patch and ran the load tests. Any exception wasn't
        occured.

        You are right. "if (validationMethod == null)" is added for performance.
        I understood that the goal of three load methods was preparing validationMethod
        variable, right?
        I want to remove unnecessary synchronization after preparing validationMethod.

        Regards,

        Show
        Takayuki Kaneko added a comment - Henri, Yes, I had an exception related this issue. Here is an exception below. [2006/04/06 16:00:07.727] [ERROR] [ValidatorForm 112 line ]com.example.validator.FieldChecks.validateId(java.lang.Object, null, null, null, null, null) org.apache.commons.validator.ValidatorException: com.example.validator.FieldChecks.validateId(java.lang.Object, null, null, null, null, null) at org.apache.commons.validator.ValidatorAction.loadValidationMethod(ValidatorAction.java:627) at org.apache.commons.validator.ValidatorAction.executeValidationMethod(ValidatorAction.java:557) at org.apache.commons.validator.Field.validateForRule(Field.java:827) at org.apache.commons.validator.Field.validate(Field.java:907) at org.apache.commons.validator.Form.validate(Form.java:174) at org.apache.commons.validator.Validator.validate(Validator.java:367) at org.apache.struts.validator.ValidatorForm.validate(ValidatorForm.java:110) at org.apache.struts.action.RequestProcessor.processValidate(RequestProcessor.java:928) at org.apache.struts.action.RequestProcessor.process(RequestProcessor.java:204) I use commons-validator 1.1.4 with Struts 1.2.7. This exception wasn't occured when I accessed with browser. It was occured under the load testing with JMeter. So, I can't attach the unit test code. In my opiniton, the three load methods needs synchronization. I modified the code with this patch and ran the load tests. Any exception wasn't occured. You are right. "if (validationMethod == null)" is added for performance. I understood that the goal of three load methods was preparing validationMethod variable, right? I want to remove unnecessary synchronization after preparing validationMethod. Regards,
        Hide
        Henri Yandell added a comment -

        The Exception you posted is a bit weird, but I'm new to Validator so maybe it
        screwed the output up. It's not possible to see what the actual error it hits is.

        Not that I don't think this should be applied - it makes sense to me and adding
        the if/synchronized statements don't break any existing unit tests.

        Show
        Henri Yandell added a comment - The Exception you posted is a bit weird, but I'm new to Validator so maybe it screwed the output up. It's not possible to see what the actual error it hits is. Not that I don't think this should be applied - it makes sense to me and adding the if/synchronized statements don't break any existing unit tests.
        Hide
        Henri Yandell added a comment -

        Following that up, really interested in seeing the non-messed up Exception. I
        still don't see what the code might be falling over on.

        Show
        Henri Yandell added a comment - Following that up, really interested in seeing the non-messed up Exception. I still don't see what the code might be falling over on.
        Hide
        Takayuki Kaneko added a comment -

        It is difficult to explain the multi-threading issue.
        But I'm going to try!

        It's my understanding that exception was occured under the following procedure.

        1) Both thread-A and thread-B have reached at line 663 of ValidatorAction.
        Because this block doesn't be protected by synchronization.

        657: private void loadParameterClasses(ClassLoader loader)
        658: throws ValidatorException {
        659:
        660: if (this.parameterClasses != null)

        { 661: return; 662: }

        663:
        664: this.parameterClasses = new Class[this.methodParameterList.size()];

        2) thread-A has reached at line 621 of ValidatorAction.

        617: private void loadValidationMethod() throws ValidatorException {
        618: if (this.validationMethod != null)

        { 619: return; 620: }

        621:
        622: try

        { 623: this.validationMethod = 624: this.validationClass.getMethod(this.method, this.parameterClasses); 625: 626: }

        catch (NoSuchMethodException e)

        { 627: throw new ValidatorException(e.getMessage()); 628: }

        3) thread-B has executed line 664. Then, parameterClasses variable has been
        initialized.

        4) thread-A has executed line 624. Then, NoSuchMethodException was occured.
        After that, ValidatorException was throwed at line 627.

        Show
        Takayuki Kaneko added a comment - It is difficult to explain the multi-threading issue. But I'm going to try! It's my understanding that exception was occured under the following procedure. 1) Both thread-A and thread-B have reached at line 663 of ValidatorAction. Because this block doesn't be protected by synchronization. 657: private void loadParameterClasses(ClassLoader loader) 658: throws ValidatorException { 659: 660: if (this.parameterClasses != null) { 661: return; 662: } 663: 664: this.parameterClasses = new Class [this.methodParameterList.size()] ; 2) thread-A has reached at line 621 of ValidatorAction. 617: private void loadValidationMethod() throws ValidatorException { 618: if (this.validationMethod != null) { 619: return; 620: } 621: 622: try { 623: this.validationMethod = 624: this.validationClass.getMethod(this.method, this.parameterClasses); 625: 626: } catch (NoSuchMethodException e) { 627: throw new ValidatorException(e.getMessage()); 628: } 3) thread-B has executed line 664. Then, parameterClasses variable has been initialized. 4) thread-A has executed line 624. Then, NoSuchMethodException was occured. After that, ValidatorException was throwed at line 627.
        Hide
        Martin Cooper added a comment -

        Your description doesn't explain why an exception would be thrown, though. Yes,
        in step 4, thread-A will be using the parameterClasses value that was set by
        thread-B in step 3, but so what? That's not going to hurt anything.

        We really do need to see an accurate stack trace. The one you have posted is
        actually not possible. There is no way that loadValidationMethod is going to
        call FieldChecks.validateId, which is what your stack trace shows. The trace
        also shows that the real problem is happening in what is presumably your own
        code - the package name is com.example.validator, which isn't part of Commons
        Validator.

        Show
        Martin Cooper added a comment - Your description doesn't explain why an exception would be thrown, though. Yes, in step 4, thread-A will be using the parameterClasses value that was set by thread-B in step 3, but so what? That's not going to hurt anything. We really do need to see an accurate stack trace. The one you have posted is actually not possible. There is no way that loadValidationMethod is going to call FieldChecks.validateId, which is what your stack trace shows. The trace also shows that the real problem is happening in what is presumably your own code - the package name is com.example.validator, which isn't part of Commons Validator.
        Hide
        Takayuki Kaneko added a comment -

        In step 3, thread-B initialized parameterClasses as an array of null.
        Then in step 4, thread-A couldn't find validateId method because
        parameterClasses was incorrect.

        If you thought that this problem was depends on the configuration, I say "No".
        Like I said, this exception didn't occur when I accessed with a browser.
        In other words, configuration was correct and this problem didn't occur
        when ValidatorAction was accessed by single thread.

        I putted a whole stack trace. The information was chipped off at line 627.
        But we can suppose the real reason by followings.

        A) ValidatorException was thrown because NoSuchMethodException was catched at
        line 626.
        B) NoSuchMethodException was thrown because parameterClasses was incorrect.
        We can see it in the stack trace.
        "FieldChecks.validateId(java.lang.Object, null, null, null, null, null)"
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        Regards,

        Show
        Takayuki Kaneko added a comment - In step 3, thread-B initialized parameterClasses as an array of null. Then in step 4, thread-A couldn't find validateId method because parameterClasses was incorrect. If you thought that this problem was depends on the configuration, I say "No". Like I said, this exception didn't occur when I accessed with a browser. In other words, configuration was correct and this problem didn't occur when ValidatorAction was accessed by single thread. I putted a whole stack trace. The information was chipped off at line 627. But we can suppose the real reason by followings. A) ValidatorException was thrown because NoSuchMethodException was catched at line 626. B) NoSuchMethodException was thrown because parameterClasses was incorrect. We can see it in the stack trace. "FieldChecks.validateId(java.lang.Object, null, null, null, null, null)" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Regards,
        Hide
        Martin Cooper added a comment -

        OK, I get it now. But rather than take the performance hit of synchronising this
        chunk of code, it would be simpler and faster to just use a local variable
        inside of the loadParameterClasses method, until all the parameter classes have
        been loaded, and then assign that to this.parameterClasses. The problem is
        happening because we are directly assigning to the member variable before the
        array has been initialised.

        Show
        Martin Cooper added a comment - OK, I get it now. But rather than take the performance hit of synchronising this chunk of code, it would be simpler and faster to just use a local variable inside of the loadParameterClasses method, until all the parameter classes have been loaded, and then assign that to this.parameterClasses. The problem is happening because we are directly assigning to the member variable before the array has been initialised.
        Hide
        Takayuki Kaneko added a comment -

        Martin,
        I'm glad that you got it.

        I considered whether I can make the synchronizatioin block would be smaller.
        But I want to keep it simple, so I added an "if (validationMethod == null)".

        I believe that this change is advisability because synchronization won't be used
        after validationMethod is prepared.

        Thanks,

        Show
        Takayuki Kaneko added a comment - Martin, I'm glad that you got it. I considered whether I can make the synchronizatioin block would be smaller. But I want to keep it simple, so I added an "if (validationMethod == null)". I believe that this change is advisability because synchronization won't be used after validationMethod is prepared. Thanks,
        Hide
        Martin Cooper added a comment -

        Fixed in the 2006-05-04 nightly build.

        I've done both. That is, applied the suggested patch as well as used a local
        variable until the parameter classes are initialised.

        Show
        Martin Cooper added a comment - Fixed in the 2006-05-04 nightly build. I've done both. That is, applied the suggested patch as well as used a local variable until the parameter classes are initialised.
        Hide
        Niall Pemberton added a comment -

        Re-openned and the set to "Resolved Fixed" again to correct "resolution" which was lost in Bugzilla --> JIRA conversion

        Show
        Niall Pemberton added a comment - Re-openned and the set to "Resolved Fixed" again to correct "resolution" which was lost in Bugzilla --> JIRA conversion

          People

          • Assignee:
            Unassigned
            Reporter:
            Takayuki Kaneko
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development