[jira] [Commented] (DIGESTER-161) Document thread-safety in javadoc of Rule class

[Eduard Papa (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/DIGESTER-161?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13197058#comment-13197058
 ] 

Eduard Papa commented on DIGESTER-161:
--

Can someone commit the new javadoc I attached, assign it to a version, and mark 
it as resolved. Thanks

> Document thread-safety in javadoc of Rule class 
> 
>
> Key: DIGESTER-161
> URL: https://issues.apache.org/jira/browse/DIGESTER-161
> Project: Commons Digester
>  Issue Type: Improvement
>Affects Versions: 3.1
>Reporter: Eduard Papa
>Priority: Trivial
>  Labels: rule, thread-safe
> Attachments: RuleJavadoc.txt
>
>   Original Estimate: 1h
>  Remaining Estimate: 1h
>
> I discovered a problem today with some code that was reusing a custom Rule in 
> multiple threads, even though each thread was creating its own digester. It 
> seems that Digester.addRule is calling rule.setDigester and if the rule is 
> shared across multiple threads, the calls to begin/end can get tangled across 
> threads. 
> It is obvious that Rules are not meant to be shared, but the javadoc 
> 
>  seems to be implying the opposite and is confusing at best. It talks about 
> the rules being stateless, even though the framework itself is changing its 
> state with rule.setDigester(digester). It further states that since all state 
> is part of the digester, the rule is safe under all cases, which is very 
> misleading.
> " ... Rule objects should be stateless, ie they should not update any 
> instance member during the parsing process. A rule instance that changes 
> state will encounter problems if invoked in a "nested" manner; this can 
> happen if the same instance is added to digester multiple times or if a 
> wildcard pattern is used which can match both an element and a child of the 
> same element. The digester object stack and named stacks should be used to 
> store any state that a rule requires, making the rule class safe under all 
> possible uses. ..."
> I think the statement above should be reworded to be more correct and avoid 
> confusion. Down the line, maybe the digester accessed by the rule should be a 
> ThreadLocal.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

[jira] [Commented] (DIGESTER-161) Document thread-safety in javadoc of Rule class

[Eduard Papa (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/DIGESTER-161?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13167834#comment-13167834
 ] 

Eduard Papa commented on DIGESTER-161:
--

The code I was referring to was actually using digester 2.x, so I don't think 
the provider method is there. It was just creating digester and adding all the 
rules. The rule that was causing the problem was a static final...hence the 
thread-safety issue.

I would have liked someone who knows more about Digester to update the javadoc 
but I'll give a trytomorrow hopefully. 

> Document thread-safety in javadoc of Rule class 
> 
>
> Key: DIGESTER-161
> URL: https://issues.apache.org/jira/browse/DIGESTER-161
> Project: Commons Digester
>  Issue Type: Improvement
>Affects Versions: 3.1
>Reporter: Eduard Papa
>Priority: Trivial
>  Labels: rule, thread-safe
>   Original Estimate: 1h
>  Remaining Estimate: 1h
>
> I discovered a problem today with some code that was reusing a custom Rule in 
> multiple threads, even though each thread was creating its own digester. It 
> seems that Digester.addRule is calling rule.setDigester and if the rule is 
> shared across multiple threads, the calls to begin/end can get tangled across 
> threads. 
> It is obvious that Rules are not meant to be shared, but the javadoc 
> 
>  seems to be implying the opposite and is confusing at best. It talks about 
> the rules being stateless, even though the framework itself is changing its 
> state with rule.setDigester(digester). It further states that since all state 
> is part of the digester, the rule is safe under all cases, which is very 
> misleading.
> " ... Rule objects should be stateless, ie they should not update any 
> instance member during the parsing process. A rule instance that changes 
> state will encounter problems if invoked in a "nested" manner; this can 
> happen if the same instance is added to digester multiple times or if a 
> wildcard pattern is used which can match both an element and a child of the 
> same element. The digester object stack and named stacks should be used to 
> store any state that a rule requires, making the rule class safe under all 
> possible uses. ..."
> I think the statement above should be reworded to be more correct and avoid 
> confusion. Down the line, maybe the digester accessed by the rule should be a 
> ThreadLocal.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

[jira] [Commented] (DIGESTER-161) Document thread-safety in javadoc of Rule class

[Simone Tripodi (Commented) (JIRA)]

[ 
https://issues.apache.org/jira/browse/DIGESTER-161?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13167804#comment-13167804
 ] 

Simone Tripodi commented on DIGESTER-161:
-

Nice to see someone deeply debugging the Digester :)

I personally avoid that kind of use of the {{Rule}} instance because of 
non-thread safety-ness nature of the {{Digester}} instance, that is why in 
digester3 I thought adding the 
{{org.apache.commons.digester3.binder.RuleProvider}} in the {{binder}} API, to 
make sure that {{Rule}} instances are not shared across different {{Digester}} 
instances.

A typical use case - maybe like yours is - you need custom inject dependencies 
in the custom {{Rule}} implementation, let's assume you are parsing an xml feed 
to ingest content inside a database:

{code}
public class MyCustomRule
extends Rule
{

private final Connection connection;

public MyCustomRule( Connection connection )
{
   this.connection = connection;
}
}
{code}

the related provider implementation would be:

{code}
public class MyCustomRuleProvider
implements RuleProvider
{

private final DataSource dataSource;

public MyCustomRuleProvider( DataSource dataSource )
{
this.dataSource = dataSource;
}

public MyCustomRule get()
{
return new MyCustomRule( dataSource.getConnection() );
}

}
{code}

then, in the {{RulesModule}}:

{code}
public class MyRulesModule
extends AbstractRulesModule
{

protected void configure()
{
forPattern( "my/rule/path" ).addRuleCreatedBy( new 
MyCustomRuleProvider( dataSourceReference ) );
}

}
{code}

said that, we have two options - I am open to both:

 * as you suggested, updating the {{Rule}} class in oder to store {{digester}} 
and {{namespaceURI}} references in {{ThreadLocals}}

or

 * updating the documentation according to current behavior - since I am not a 
native English speaker, I would like you to provide a patch to apply, please :P

Thanks for participating!
-Simo 

> Document thread-safety in javadoc of Rule class 
> 
>
> Key: DIGESTER-161
> URL: https://issues.apache.org/jira/browse/DIGESTER-161
> Project: Commons Digester
>  Issue Type: Improvement
>Affects Versions: 3.1
>Reporter: Eduard Papa
>Priority: Trivial
>  Labels: rule, thread-safe
>   Original Estimate: 1h
>  Remaining Estimate: 1h
>
> I discovered a problem today with some code that was reusing a custom Rule in 
> multiple threads, even though each thread was creating its own digester. It 
> seems that Digester.addRule is calling rule.setDigester and if the rule is 
> shared across multiple threads, the calls to begin/end can get tangled across 
> threads. 
> It is obvious that Rules are not meant to be shared, but the javadoc 
> 
>  seems to be implying the opposite and is confusing at best. It talks about 
> the rules being stateless, even though the framework itself is changing its 
> state with rule.setDigester(digester). It further states that since all state 
> is part of the digester, the rule is safe under all cases, which is very 
> misleading.
> " ... Rule objects should be stateless, ie they should not update any 
> instance member during the parsing process. A rule instance that changes 
> state will encounter problems if invoked in a "nested" manner; this can 
> happen if the same instance is added to digester multiple times or if a 
> wildcard pattern is used which can match both an element and a child of the 
> same element. The digester object stack and named stacks should be used to 
> store any state that a rule requires, making the rule class safe under all 
> possible uses. ..."
> I think the statement above should be reworded to be more correct and avoid 
> confusion. Down the line, maybe the digester accessed by the rule should be a 
> ThreadLocal.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira