Pages transformed by SpringSecurityWorker are not thread-safe

[ghost]

Tapestry does not use a page pool anymore, but only one instance of a page. The introduced _$token field can be accessed by multiple concurrent threads.

Use the following in MethodAdvice.advise to store the InterceptorStatusToken:

ComponentResources componentResources = invocation.getInstanceContext().get(ComponentResources.class);

componentResources.storeRenderVariable(STATUS_TOKEN, statusToken);

InterceptorStatusToken statusToken = (InterceptorStatusToken) componentResources.getRenderVariable(STATUS_TOKEN);

[antalk]

Isn't this handled by the Tapestry core itself ? From Howard's blog:

"What's going on in trunk (Tapestry 5.2 alpha) right now is extrapolating that concept from just persistent fields to all mutable fields. Every access to every mutable field in a Tapestry page is converted, as part of the class transformation process, into an access against a per-thread Map of keys and values. Each field gets a unique identifying key. The Map is discarded at the end of the request.

The end result is that a single page instance can be used across multiple threads without any synchronization issues and without any field value conflicts. "

After checking out on Tapestry's mailinglist, the answer is that the field will be ThreadLocalized and thus not have concurrency issues. See http://tapestry.1045711.n5.nabble.com/plastic-adding-fields-to-pages-td5718316.html#a5718318

[ghost]

Ok, I looked into this again, and discovered the following, at least using Tapestry 5.3.6 (coming from 5.0.18), so I don't know what the behaviour was on 5.3.x.

As stated UnclaimedFieldWorker is executed after SpringSecurityWorker, but the _statusToken field is not in the list of fields transformed by UnclaimedFieldWorker. As far as I can see this is, because PlasticClassImpl.introduceField doesn't add it to the PlasticClassImpl.fields list, which is initialized on class creation. Though it is included in PlasticClass.getFieldNames(). So this is either a bug in Tapestry or it is required to add a Conduit yourself when introducing fields ...

[antalk]

After inspection of the PlasticClassImpl it looks like it does actually. See:

public PlasticField introduceField(String className, String suggestedName) { check();

    assert PlasticInternalUtils.isNonBlank(className);
    assert PlasticInternalUtils.isNonBlank(suggestedName);

    String name = makeUnique(fieldNames, suggestedName);

    // No signature and no initial value

    FieldNode fieldNode = new FieldNode(ACC_PRIVATE, name, PlasticInternalUtils.toDescriptor(className), null, null);

    classNode.fields.add(fieldNode);     <--------- here the introduced field gets added to the classnodes field list

And then in the constructor:

fields = new ArrayList(classNode.fields.size()); <-- here the added fields to the classnode gets assigned

    for (FieldNode node : classNode.fields)
    {
        fieldNames.add(node.name);

        // Ignore static fields.

        if (Modifier.isStatic(node.access))
            continue;

        // When we instrument the field such that it must be private, we'll get an exception.

        fields.add(new PlasticFieldImpl(this, node));     <-- and added for transformation
    }

[ghost]

Yes, but this does only include the fields already defined in the class as the constructor is called first. The fields introduced are missed, because, as I understand, the sequence is the following:

1. Construct a PlasticClassImpl (existing fields are added)
2. Pass this to the ClassTransformWorker2 chain
3. SpringSecurityWorker adds token field (added to fieldNames and classNode.fields, but not to fields)
4. UnclaimedFieldWorker transforms all fields returned by PlasticClassImpl.getFields(), which misses the introduced field I debugged it and the token field was not among the fields UnclaimedFieldWorker transforms.

[antalk]

Yes, it looks like that's the way it works. I have posted the question on the mailinglist. Let's see what the experts have to say about this.

[antalk]

I'm going to re-open this after discussion on the mailinglist:

http://tapestry.1045711.n5.nabble.com/BUG-UnclaimedFieldWorker-td5718537.html