Bug 65424 - deadlock
Summary: deadlock
Status: RESOLVED FIXED
Alias: None
Product: Ant
Classification: Unclassified
Component: Core (show other bugs)
Version: 1.10.8
Hardware: PC Linux
: P2 major (vote)
Target Milestone: 1.10.12
Assignee: Ant Notifications List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-02 12:16 UTC by Eric Delaunay
Modified: 2021-09-30 05:37 UTC (History)
0 users



Attachments
deadlock stracktrace (13.71 KB, text/x-log)
2021-07-02 12:16 UTC, Eric Delaunay
Details
Patch to fix deadlock in Netbeans ANT console (1.66 KB, patch)
2021-07-02 12:26 UTC, Eric Delaunay
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Delaunay 2021-07-02 12:16:51 UTC
Created attachment 37933 [details]
deadlock stracktrace

When running ANT in NetBeans 12.4 (ANT 1.10.8) along with AntContrib's <for> task in a rather complex build script, I can experiment deadlocks every time I enable parallel run in my <for> tasks (parallel="true" threadCount="${N}").

I think it is related to the way NetBeans intercept log events in its NbBuildLogger class.(o.apache.tools.ant.module/src-bridge/org/apache/tools/ant/module/bridge/impl/NbBuildLogger.java in NetBeans sources).

The logger wraps each event into a NetBeans' AntEvent class then call Task.getRuntimeConfigurableWrapper() during log to extract tasks infos. However, getRuntimeConfigableWrapper() method might invoke IntrospectionHelper.getHelper(...) from its maybeConfigure method, and the IntrospectionHelper might trigger class loading from an AntClassLoader, which, in turn, will log a message.

When ANT tasks are run in parallel threads, it is prone to deadlock due to locking mechanism in AntClassLoader.loadClass and IntrospectionHelper.getHelper which are synchronized methods. Especially, the latter takes a lock during introspection phase which can trigger Class loading and messageLogged from AntClassLoader!
See the stack trace in attachment.

Also see this NetBeans issue : https://issues.apache.org/jira/browse/NETBEANS-5368
Comment 1 Eric Delaunay 2021-07-02 12:26:45 UTC
Created attachment 37934 [details]
Patch to fix deadlock in Netbeans ANT console

I found a way to circumvent the deadlock: don't hold the lock during introspection but only when filling the HELPERS map (HELPERS is a Hashtable and is already "synchronized" at method level).

The synchronized (HELPERS) {...} block put the result in the map if not already in (eg. another thread requested an InstrospectionHelper on the same class).
Comment 2 Jaikiran Pai 2021-07-06 12:01:35 UTC
This does look like a genuine deadlock. Thank you for the proposed patch. I'll take a closer look at it and see if we need something more or if this is enough.
Comment 3 Stefan Bodewig 2021-09-19 10:15:11 UTC
getHelpers has always been synchronized. The main reason for this has been performance rather than thread-safety. Back in 2000 when the class was written, reflection was incredibly expensive and we simply wanted to avoid reflecting on a class unnecessarily.

My initial thought was "time to finally start using something more modern than Hashtable" and indeed at first glance doing away with synchronization and using ConcurrentHashMap compute(IfAbsent) could work. But that may cause situations where we run into deadlocks as well if loading the helper for a class somehow triggers loading a helper for the same class once again. Also the "same class different classloader" logic might need some thought.

Your patch may cause us to run reflection on the same class more often than strictly required but I think this is acceptable today. It does lack the "is this the same classloader" check, though, so you should add oh.bean == ih.bean in addition to oh != null inside of the new block (and add braces, while you are at it :-)).

Right now I cannot see any additional problems the patch could cause.

Thank you.
Comment 4 Jaikiran Pai 2021-09-26 04:44:40 UTC
I wanted to include this fix in an upcoming release of Ant 1.10.x. So I went ahead and used this patch (and modified it a bit) and committed it to upstream.

Stefan, it would be good to have your review on the commit I did. Overall, the patch is same as what is proposed here with some minor changes:
 - I moved the project check to the start of the method (just to make it clear that we don't cache the helper in such cases)
 - added some code comments
 - Removed the "synchronized" on clearCache() since it's no longer needed because we are now using the lock on the "HELPERS" instance instead of on the "IntrospectionHelper".

ED-Apache, I'm not sure if you are a first time contributor to Ant or if you have contributed previously. If this is your first time, please let us know what name (first name, last name) you would like us to add in our contributors list that we maintain in the project.
Comment 5 Stefan Bodewig 2021-09-26 10:00:19 UTC
your change looks good, Jaikiran, thank you.
Comment 6 Eric Delaunay 2021-09-27 15:35:13 UTC
Thanks to all. I verified it works well in my NetBeans instance (with local build of latest commit 433819ffb6e18fbf0c81dda150527cb7d05a1ed7 from master).
It is my first contribution to ANT. Glad to help.
Best regards.

PS: I updated my account infos.
Comment 7 Jaikiran Pai 2021-09-30 05:37:51 UTC
Thank you for testing the fix. We have added your name to our contributors list. 1.10.12 release voting is currently in progress and if it goes through then this fix should be available in that release soon.