Re: concurrency problem with #include as template

Yeah, you're absolutely right Keats.  So is Carl.  I'm headed out of  
town, but will work on a fix and commit it next week when I return.

I'm also considering taking my laptop and the WM sources with me.  I  
want to play with the parser.  I hate our whitespace handing rules  
and have some ideas on making it better.  The long plane flights will  
probably give me enough time to work it out.

I've also discovered a few classes that should probably implement the  
Visitable interface that don't.  I've been working on a little static  
analysis tool to find usages of variables and such in templates and  
link them back up to known object types... as a way to find and head  
off template-level bugs.  I had to revert to some reflection trickery  
while visiting a few Template node types.

eric

On Feb 6, 2006, at 2:12 PM, Keats Kirsch wrote:

> This is a good catch.  It's important to understand how directives  
> work
> in WM, so we can avoid this kind of problem in the future.  (I know I
> need to review the directives I have worked on in light of this.)
> Here's my understanding of the process:
>
> At build time an instance of the XXXDirective class is created for  
> each
> reference of the directive in the template. The compiled template is
> cached and may be accessed by multiple threads, so any state  
> information
> in a directive should generally not be modified during the expansion
> (evaluation) phase.  You could do this using synchronized access,  
> but in
> general you're better off keeping this state information in a separate
> context (Context, Session, BrokerLocal, etc.).
>
> Somebody please correct me if I've gotten it wrong.
>
> Keats
>
> Carl H. Sayres wrote:
>
>> I think I solved it! But I'm new to the inner workings of  
>> WebMacro, so please let me know if I've overlooked something.
>>
>> The problem appears to be in the IncludeDirective.write() method.  
>> The IncludeDirective class has two instance fields:
>>    protected Macro _macFilename;
>>    protected String _strFilename;
>>
>> In the case of an included file, the filename is stored in  
>> _strFilename. But if the included filename comes from a macro,  
>> then the macro name is stored in _macFilename.
>>
>> The write() method has no problem with the single file case, using  
>> _strFilename which never changes. But in the case of a macro, write 
>> () stores the name of the file it is trying to include in  
>> _strFilename.  If two threads do this simultaneously, one will  
>> clober the other by overwriting _strFilename.
>>
>> My first solution was to synchronize the write() method. This does  
>> solve the problem, but it has negatve performance implications.
>>
>> My second solution is to use a local variable in write() to store  
>> the filename that we are including, passing through anything  
>> previously stored in _strFilename, to preserve the simple file  
>> include case.
>>
>> Here is my new write() method:
>>
>>    public void write (FastWriter out, Context context) throws  
>> PropertyException, IOException
>>    {
>>       String localFilename = _strFilename;
>>       Broker broker = context.getBroker();
>>
>>        // the filename arg passed to us was a Macro, so
>>        // evaluate and check it now
>>        if (_macFilename != null)
>>        {
>>            localFilename = _macFilename.evaluate(context).toString();
>>            if (localFilename == null || localFilename.length() == 0)
>>            {
>>                throw makePropertyException("Filename cannot be  
>> null or empty");
>>            }
>>        }
>>
>>        if (_log.loggingDebug() && context.getCurrentLocation 
>> ().indexOf(localFilename) > -1)
>>        {
>>            // when in debug mode, output a warning if a template  
>> tries to include itself
>>            // there are situtations where this is desired, but  
>> it's good to make
>>            // the user aware of what they're doing
>>            _log.warning(context.getCurrentLocation() + " includes  
>> itself.");
>>        }
>>
>>        // this should only be true if StrictCompatibility is set  
>> to false
>>        // and "as <something>" wasn't specified in the arg list
>>        if (_type == TYPE_DYNAMIC)
>>            _type = guessType(broker, localFilename);
>>
>>        if (_log.loggingDebug())
>>        {
>>            _log.debug("Including '" + localFilename + "' as "
>>                    + ((_type == TYPE_MACRO) ? "MACRO"
>>                    : (_type == TYPE_TEMPLATE) ? "TEMPLATE"
>>                    : (_type == TYPE_TEXT) ? "TEXT"
>>                    : "UNKNOWN.  Throwing exception"));
>>        }
>>
>>        Object toInclude = getThingToInclude(broker, _type,  
>> localFilename);
>>        switch (_type)
>>        {
>>            case TYPE_MACRO:
>>                // during runtime evaluation of a template,
>>                // a TYPE_MACRO doesn't really work as expected.
>>                // we logged a warning above in build(), but
>>                // we still need to write it out as a template
>>                // so just fall through
>>            case TYPE_TEMPLATE:
>>                out.write(((Template) toInclude).evaluateAsBytes 
>> (out.getEncoding(), context));
>>                break;
>>
>>            case TYPE_TEXT:
>>                // static types are strings
>>                out.write(toInclude.toString());
>>                break;
>>
>>            default:
>>                // should never happen
>>                throw makePropertyException("Unrecognized file  
>> type: " + _type);
>>        }
>>    }
>>
>> The most important change is the third line:
>>       String localFilename = _strFilename;
>> Then every other place where _strFilename was referenced has been  
>> changed to localFilename.
>> This has no effect for the single included file case. In the case  
>> of a macro:
>>        if (_macFilename != null)
>>        {
>>            localFilename = _macFilename.evaluate(context).toString();
>>            if (localFilename == null || localFilename.length() == 0)
>>            {
>>                throw makePropertyException("Filename cannot be  
>> null or empty");
>>            }
>>        }
>> then localFilename is set to the corret value  within  each  
>> thread . And the threads can't  overwrite  each other's data.
>>
>> Oh, yes, and my test now does not fail!
>>
>> Let me know if you agree with my analysis.
>>
>> Carl
>
>
>
>
>
>
> -------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc. Do you grep through  
> log files
> for problems?  Stop!  Download the new AJAX search engine that makes
> searching your log files as easy as surfing the  web.  DOWNLOAD  
> SPLUNK!
> http://sel.as-us.falkag.net/sel? 
> cmd=lnk&kid=103432&bid=230486&dat=121642
> _______________________________________________
> Webmacro-user mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/webmacro-user



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642