Re: [WebMacro-user] concurrency problem with #include as template

[Carl H. S. <ca...@da...>]

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