[Bug 65424] New: deadlock

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=65424

            Bug ID: 65424
           Summary: deadlock
           Product: Ant
           Version: 1.10.8
          Hardware: PC
                OS: Linux
            Status: NEW
          Severity: major
          Priority: P2
         Component: Core
          Assignee: notifications@ant.apache.org
          Reporter: ed-apache@dejapris.eu
  Target Milestone: ---

Created attachment 37933
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=37933&action=edit
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

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 65424] deadlock

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=65424

--- Comment #3 from Stefan Bodewig <bo...@apache.org> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 65424] deadlock

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=65424

--- Comment #6 from Eric Delaunay <ed...@dejapris.eu> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 65424] deadlock

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=65424

--- Comment #7 from Jaikiran Pai <ja...@apache.org> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 65424] deadlock

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=65424

--- Comment #5 from Stefan Bodewig <bo...@apache.org> ---
your change looks good, Jaikiran, thank you.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 65424] deadlock

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=65424

--- Comment #2 from Jaikiran Pai <ja...@apache.org> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 65424] deadlock

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=65424

--- Comment #1 from ED-Apache <ed...@dejapris.eu> ---
Created attachment 37934
  --> https://bz.apache.org/bugzilla/attachment.cgi?id=37934&action=edit
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).

-- 
You are receiving this mail because:
You are the assignee for the bug.

[Bug 65424] deadlock

[bu...@apache.org]

https://bz.apache.org/bugzilla/show_bug.cgi?id=65424

Jaikiran Pai <ja...@apache.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |1.10.12
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #4 from Jaikiran Pai <ja...@apache.org> ---
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.