Review of fix for JBWS-2304

[dlofthouse]

I have been working on the following issue: -

https://jira.jboss.org/jira/browse/JBWS-2304

The fundamental problem is that the ThreadLocals are not being cleared when the client request completes.

The initial fix has been to clear the ThreadLocals once the MessageContextAssociation stack is empty with a couple of exceptions.

1 - When the MessageContextJAXWS pivots then a flag is needed to avoid clearing the thread local.

2 - For message style endpoints the ThreadLocal of the Document is used for further transitions if the SOAPMessage is subsequently accessed, for this the XMLSource now caches the Document and sets it on the DOMUtils.

http://fisheye.jboss.com/changelog/JBossWS/?cs=8242
http://fisheye.jboss.com/changelog/JBossWS/?cs=8245

Overall I think the mechanism to cache the Document and DocumentBuilder should be re-factored to possibly associate it with the MessageContext and the message itself so that it does not need to be in it's own ThreadLocal with a potential for leaks - however I am not sure if such a bug change would be suitable at the moment.

Anyway I just wanted to see if you have any additional thoughts on this fix before I commit.

[ropalka]

Overall I think the mechanism to cache the Document and DocumentBuilder should be re-factored to possibly associate it with the MessageContext and the message itself so that it does not need to be in it's own ThreadLocal with a potential for leaks - however I am not sure if such a bug change would be suitable at the moment.

Anyway I just wanted to see if you have any additional thoughts on this fix before I commit.

Hi Darran,

to be honest I don't like our current DOMUtils ThreadLocals capability. ThreadLocals holding thread-specific fields should only be used when there is no other good option available. The following are top two reasons to avoid ThreadLocals:

The use of thread-specific variables tends to hide parameters that influence behaviour and can make it harder to check for errors or leakage. In this sense, thread-specific variables present the same, although less extreme, traceability problems as static global variables.
Use of thread-specific variables can detract from reusability by increasing code dependencies.

In other words if you will refactor our DOMUtils and will remove ThreadLocals dependent code, I will really like your solution ;)

Richard

[dlofthouse]

Thanks Richard,

Would it be Ok to go ahead with my initial fix to begin with and then I will work on the a complete solution including refactoring next - one issue that will need to resolved is that because DOMUtils is in common it does have some use with the other three stacks.

[ropalka]

Would it be Ok to go ahead with my initial fix to begin with and then I will work on the a complete solution including refactoring next

Yes, no problem.

- one issue that will need to resolved is that because DOMUtils is in common it does have some use with the other three stacks.

AFAICS DOMUtils ThreadLocal capability is used in native stack only ;)

[dlofthouse]

AFAICS DOMUtils ThreadLocal capability is used in native stack only ;)

I will double check but I think I found one way it does get called when used by Metro to read a deployment descriptor - again leaving the Document and DocumentBuilder associated with the Thread.