Re: concurrency problem with #include as template

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