Bug 329126

Comment 1 Abraham Nevado 2010-10-29 20:25:57 EDT

Created attachment 182091 [details]
Thread dump

Comment 2 Abraham Nevado 2010-10-29 20:36:44 EDT

As you can see in the thread dump is issue is not directly originated by aspectj but triggered. It seems to me the SaxParser from IBM J9 is trying to load the parser and instead of doing it through the extClassLoader it uses ApplicationClassLoader that is blocked by other thread that is trying to load another resource from the extClassLoader which in turns is already blocked... Here the deadlock arises.

I believe this problem is caused because from the Sax implementation loaded from extClassLoader, it tries to load from a lower ClassLoader such us the CompoundClassLoader... 

In order to bypass this circumstance I propose to add some change in the way the parser is chosen from the aspectj implementation based on a SystemProperty that can be added in case not working easily without the need of changing aop.xml....

At DocumentParser.java:

       /**
	 * To avoid deadlock under certain circunstances PR 182091
	 */
	
	private final static boolean USE_SAX_PARSER_FACTORY = System.getProperty("org.aspectj.useSaxParser", "false").equals("true");
	
...

	private static XMLReader getXMLReader() throws SAXException, ParserConfigurationException {
		XMLReader xmlReader = null;

		if(USE_SAX_PARSER_FACTORY){
			xmlReader = SAXParserFactory.newInstance().newSAXParser().getXMLReader();
		}else{
			/* Try this first for Java 5 */
			try {
				xmlReader = XMLReaderFactory.createXMLReader();
			}
	
			/* .. and ignore "System property ... not set" and then try this instead */
			catch (SAXException ex) {
				xmlReader = SAXParserFactory.newInstance().newSAXParser().getXMLReader();
			}
		}
		return xmlReader;
	}

What do you think?

Thanks in advance, 

Best regards.

P.D.: A tentative patch is attached.

Comment 3 Abraham Nevado 2010-10-29 20:37:51 EDT

Created attachment 182092 [details]
A tentative patch

Comment 4 Abraham Nevado 2010-10-29 23:10:31 EDT

Tentive patch does not work although it reduces the deadLock frequency, it is still there.

The only valid workarround, we are testing right now, is caching the parsed XML instead of loading it every single time...

I will update the case as soon as we have robust results and data.

Comment 5 Abraham Nevado 2010-10-31 15:48:27 EDT

After several tries I found the only work around is to cache XML parsing, by doing this first you avoid the deadlock condition and by the way you enhance LTW performance. 

I propose adding a new configuration variable toggling caching one, when you are using just aop.xml, this will boost performance and will avoid possible deadlocks.

What do you think?

I am coding the patch...

Comment 6 Abraham Nevado 2010-11-01 16:18:47 EDT

I am attaching the final patch that worked for us at production. The idea is to toggle cache AOP strategy through a System variable as it makes no sense to use AOP to define if AOP should be cached.

To enable it:

set the next property org.aspectj.weaver.loadtime.configuration.cache to true:

-Dorg.aspectj.weaver.loadtime.configuration.cache=true

Hope helps,

Best regards.

Comment 7 Abraham Nevado 2010-11-01 16:20:11 EDT

Created attachment 182172 [details]
Final patch

The patch that enables by the way AOP caching which boost LTW time specially when loading in an environment when hundreds of JSPs are available.

Comment 8 Andrew Clement 2010-11-01 19:49:55 EDT

Thanks for the patch, committed.  Feels like we could eventually turn this option on by default, I can't think of many downsides - unless there are a large number of xml files (or a small number of large xml files), as they'll chew up memory indefinetly.

I could perhaps even go for an alternative syntax to XML - key/value files or somesuch.  These aop.xml files aren't usually all that complicated.

Comment 9 Abraham Nevado 2011-02-28 14:43:00 EST

The cache has reduced deadlock frequency, however still getting in some environments. We have created a a really lightweight, reduced features XML parser to avoid SAX usage and thus the potential deadlock.

Please find attached the patch.

Regards.

Comment 10 Abraham Nevado 2011-02-28 14:45:27 EST

Created attachment 189986 [details]
LightWeight XML parser to avoid SAX usage.

Comment 11 Andrew Clement 2011-02-28 19:04:54 EST

that was quick!

As we are about to head into the Release Candidate for 1.6.11, I was wondering about making the new XML parsing opt-in rather than the default.  What do you think?  Just having a system property that means you could use it - if it proves robust over a few months we can switch it to default?

Comment 12 Abraham Nevado 2011-03-01 07:38:24 EST

Of course. It is the best approach: here you are the patch.

Comment 13 Abraham Nevado 2011-03-01 07:38:57 EST

Created attachment 190032 [details]
Version 2, using SystemProperty

Comment 14 Andrew Clement 2011-03-01 13:15:16 EST

Thanks for the updated patch, I'm trying it out now.  The test runs are clean with it like it is.  Then I forced the parser and caching ON to try a test run with the new stuff enabled, currently seeing at least 15 failures.  Some like this:

junit.framework.AssertionFailedError: 
  expecting output:
advice fired class Dode
  but found output:
[WeavingURLClassLoader] warning parse definitions failed -- (Exception) Parsing error. Excpected > but got: e
Parsing error. Excpected > but got: e
java.lang.Exception: Parsing error. Excpected > but got: e
	at org.aspectj.weaver.loadtime.definition.LightXMLParser.skipComment(LightXMLParser.java:373)
	at org.aspectj.weaver.loadtime.definition.LightXMLParser.parseNode(LightXMLParser.java:331)
	at org.aspectj.weaver.loadtime.definition.LightXMLParser.parseFromReader(LightXMLParser.java:70)
	at org.aspectj.weaver.loadtime.definition.SimpleAOPParser.parse(SimpleAOPParser.java:66)
	at org.aspectj.weaver.loadtime.definition.DocumentParser.parse(DocumentParser.java:115)
	at org.aspectj.weaver.loadtime.ClassLoaderWeavingAdaptor.parseDefinitions(ClassLoaderWeavingAdaptor.java:258)

But perhaps more worryingly some tests are not giving me an exception and yet producing the wrong output - as if an aspect has been missed.  (Example would be OverweavingTests.testCallsTjp1)

Comment 15 Andrew Clement 2011-03-01 15:19:52 EST

all committed, thanks!