You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by bu...@apache.org on 2006/04/24 15:49:52 UTC
DO NOT REPLY [Bug 39393] New: - ValidatorAction needs thread-safe
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
Summary: ValidatorAction needs thread-safe
Product: Commons
Version: unspecified
Platform: All
OS/Version: All
Status: NEW
Severity: major
Priority: P2
Component: Validator
AssignedTo: commons-dev@jakarta.apache.org
ReportedBy: kanekotky@yahoo.co.jp
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);
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
------- Additional Comments From bayard@apache.org 2006-04-28 04:34 -------
Typical POST then DOH moment. Extra defensive coding stops the synchronized block being entered all
the time.
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
------- Additional Comments From kanekotky@yahoo.co.jp 2006-05-04 04:03 -------
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,
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
------- Additional Comments From kanekotky@yahoo.co.jp 2006-04-30 06:07 -------
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,
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
martinc@apache.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution| |FIXED
------- Additional Comments From martinc@apache.org 2006-05-04 04:27 -------
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.
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
------- Additional Comments From kanekotky@yahoo.co.jp 2006-05-04 00:56 -------
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,
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
------- Additional Comments From martinc@apache.org 2006-05-03 17:02 -------
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.
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
------- Additional Comments From bayard@apache.org 2006-05-03 00:24 -------
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.
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
gudnabrsam@yahoo.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Summary|ValidatorAction needs |[validator] ValidatorAction
|thread-safe |needs thread-safe
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
------- Additional Comments From bayard@apache.org 2006-04-28 06:40 -------
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 :)
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
------- Additional Comments From bayard@apache.org 2006-04-28 04:29 -------
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.
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
------- Additional Comments From kanekotky@yahoo.co.jp 2006-05-03 07:26 -------
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.
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
------- Additional Comments From martinc@apache.org 2006-05-04 03:20 -------
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.
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 39393] - [validator] ValidatorAction needs thread-safe
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=39393>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=39393
------- Additional Comments From bayard@apache.org 2006-05-03 00:03 -------
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.
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org