[validator] ValidatorAction needs thread-safe

• Key: VALIDATOR-89
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Takayuki Kaneko
• Fix Version/s: 1.3.1 Release

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);

Comments

[Henri Yandell]

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.

[Henri Yandell]

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

[Henri Yandell]

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

[Takayuki Kaneko]

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,

[Henri Yandell]

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.

[Henri Yandell]

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.

[Takayuki Kaneko]

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.

[Martin Cooper]

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.

[Takayuki Kaneko]

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,

[Martin Cooper]

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.

[Takayuki Kaneko]

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,

[Martin Cooper]

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.

[Niall Pemberton]

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