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

["Simone Tripodi (Commented) (JIRA)" <ji...@apache.org>]

    [ 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<MyCustomRule>
{

    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 <http://commons.apache.org/digester/apidocs/org/apache/commons/digester3/Rule.html> 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