This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 24506 - Executor not notifying on completion
Summary: Executor not notifying on completion
Status: CLOSED FIXED
Alias: None
Product: projects
Classification: Unclassified
Component: Ant (show other bugs)
Version: 3.x
Hardware: PC Windows ME/2000
: P3 blocker (vote)
Assignee: David Konecny
URL:
Keywords:
: 25528 (view as bug list)
Depends on:
Blocks:
 
Reported: 2002-06-06 23:50 UTC by Rich Unger
Modified: 2003-10-03 13:01 UTC (History)
3 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
proper fix (1.53 KB, patch)
2002-07-22 11:43 UTC, David Konecny
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rich Unger 2002-06-06 23:50:47 UTC
I'm calling into the non-API parts of the ant module, but 
this problem seems like a problem with the general 
Execution API.  I've got an ant build file in the root of 
my repository, and the code below correctly invokes it.  
The task appears in the Processes section of the Runtime 
tab in the explorer.  When the task completes, the task 
correctly disappears from the Processes section.  However, 
the Task object never sees itself as being complete (i.e. 
isFinished() never returns true).

Here's the code:

        // get the build file
        FileObject foBuildScript = Repository.getDefault
().findResource(BUILD_SCRIPT);
        if( foBuildScript == null )
        {
            return null;
        }

        DataObject dObj = null;
        try
        {
            dObj = DataObject.find(foBuildScript);
        }
        catch( DataObjectNotFoundException ex )
        {
            return null;
        }

        AntProjectCookie cookie = (AntProjectCookie)
            dObj.getCookie(AntProjectCookie.class);

        TargetExecutor executor = new TargetExecutor( 
cookie, ANT_TARGET );
        ExecutorTask task = null;

        try
        {
            task = executor.execute();
        }
        catch( IOException ex )
        {
            return null;
        }

        while( !task.isFinished() )
        {
           try {
             Thread.sleep(1000);
           } catch( Exception ex ) {}
           System.err.println("not finished yet...");
        }
Comment 1 David Strupl 2002-06-10 13:45:59 UTC
First round of evaluation:

1. Busy waiting for finishing using while (...) is not good. Please try 
  a. task.waitFinished()
  b. task.addTaskListener(new YourListener() { ... } );

2. There might be a bug in resulting from the fact that method
org.openide.util.Task.isFinished() is not synchronized. According to
java memory model the value written to the variable might not be seen
by the thread calling only isFinished() (but the risk of observing
this is relatively small (multiprocessor machines)).

3. TargetExecutor.execute returns its own task -- any possibility of a
bug over there?

I am adding Jesse on Cc: (ant module owner). Jesse any hints?

Also Rich could you please try 1a and 1b before we move on? If you
want to give it a try and find for yourself whether 2. applies you can
simply make isFinished method synchronized and check whether something
has changed.

Please let us know whether you succeeded with either 1a, 1b or 2.
Thanks for your time.
Comment 2 Jesse Glick 2002-06-10 15:55:43 UTC
I am not the Ant module owner - that is David Konecny. It seems likely
there is a bug in TargetExecutor; its wrapper task does not override
isFinished.

Re. #1 - yes, your busy loop is not correct style, use one of the
options David S. suggests.

Re. #2 - I don't think there is a problem in isFinished() not being
synchronized; the methods which change it are synchronized I think, so
they should cross a memory barrier and cause the value to be refreshed
on all processors. AFAIK.
Comment 3 David Strupl 2002-06-10 16:10:50 UTC
Thanks for reassigning to the proper owner.

Add 2#: Not true - you have to synchronize even read to a variable
because if thread does not cross monitorEnter or monitorExit it does
not have to synchronize its working memory with the main memory. But
on single processor systems this is not an issue. Further reading:
Java Language Specification, Chapter 17.6. Or more popular: J. Bloch:
Effective Java, Chapter 9.
Comment 4 David Strupl 2002-06-10 16:13:58 UTC
My last comment was for Jesse but he was not on Cc. I have added you
but if you are no longer interested please remove yourself. Sorry for
the confusion.
Comment 5 Jesse Glick 2002-06-10 16:59:05 UTC
I am subscribed to issues@ant, no need to keep me on a separate CC.

Re. reading a boolean field w/o synch: I think the problem is that the
reading thread might not see the write to the field *in the same
order* as other things are done, if it makes no monitor entries. But
it will *soon* see the new value: the thread cannot simply ignore the
contents of main memory forever. At least that is how I read the
relevant sections of the JLS.

In other words, if the contract of Task.isFinished is that while the
task is not finished, it will return false, and at some point after
the task finishes, it will return true, then the contract is satisfied
IMHO.
Comment 6 David Strupl 2002-06-10 17:17:30 UTC
Sorry, but your "soon" is not defined in the JLS. It can be 1 hour or
whatever. The thread does not have to synchronize its working memory
with the main memory in absence of synchronization. In single
processor JVM impls it does behave like you expected but there can be
perfectly compliant JVM that would *never* fetch the boolean from the
main memory. I said it is a rare case. Unfortunatelly your statement
that "the thread cannot simply ignore the contents of main memory
forever" is not true - it can (in case it does not go across
synchronization).
Comment 7 Jesse Glick 2002-06-10 17:30:35 UTC
I'm not sure whether a compliant JVM can in fact ignore main memory
forever, but certainly you are right that it could do so for an hour
(however unlikely this may be).

It's not entirely clear to me why you would even use isFinished() at
all, actually, unless you were already somehow inside a synchronized
block for the task. It doesn't seem very useful because its contract
is rather weak.
Comment 8 Rich Unger 2002-06-10 18:36:30 UTC
My original code did call waitFinished().  I was only busy 
waiting for debugging purposes.

I will try the listener method as well.
Comment 9 Rich Unger 2002-06-10 19:10:58 UTC
Okay, the listener method did not work, either.
Comment 10 David Konecny 2002-06-11 13:40:14 UTC
Bug in Ant module. The task which is returned from TargetExecutor is 
wrapper task. Unfortunatelly this wrapper task is never started, 
therefore it cannot report that the wrapped task has already finished. 
The fix is simple.

Rich, could you please verify that it works? The fix was just commited 
into CVS. I tested it and it worked fine for me.

Fixed in file:
Checking in src/org/apache/tools/ant/module/run/TargetExecutor.java
new revision: 1.22; previous revision: 1.21
Comment 11 Rich Unger 2002-06-11 19:55:07 UTC
Yep, that worked.  Thanks for the speedy bug fix.  It 
really helped me hit my schedule!
Comment 12 David Konecny 2002-07-22 11:38:01 UTC
This fix caused regression, see issue 25528.
Comment 13 David Konecny 2002-07-22 11:38:55 UTC
*** Issue 25528 has been marked as a duplicate of this issue. ***
Comment 14 David Konecny 2002-07-22 11:43:10 UTC
Created attachment 6832 [details]
proper fix
Comment 15 David Konecny 2002-07-22 11:45:42 UTC
Attached is correct patch for this issue. Jesse, do you think I should 
try to integrate this into NB3.4?
Comment 16 Marek Grummich 2002-07-22 12:26:06 UTC
Set target milestone to TBD
Comment 17 Jesse Glick 2002-07-22 18:14:19 UTC
Well I think issue #25528 should not be left open in release34 branch,
it is a serious problem affecting users - compared to #24506 which has
no impact on end-users, only module developers using the Ant module (a
handful at most). Either the original patch for this issue should be
reverted or this revised patch should be applied.

The patch looks reasonable for release34, I suppose, but there is no
information in this or the other report explaining why the hang
occurred, and why this revised patch solves it. Who was blocking in
#25528? Some more details would be helpful.
Comment 18 David Konecny 2002-07-23 13:12:28 UTC
First about the problem:

When Ant target is executed, the ExecutorTask is created. This 
ExecutorTask is wrapped into the WrapperExecutorTask and instance of 
WrapperExecutorTask is returned from TargetExecutor.execute(). 
WrapperExecutorTask does nothing - it simply waits till the wrapped 
executor task is finished. WrapperExecutorTask is ExecutorTask and so 
it must be started. This was the problem filed as this issue - task 
returned from TargetExecutor.execute() was never started and therefore 
its isFinished() was never true (although that Ant target wrapped in 
this task has already finished).

And so I fixed it by starting the WrapperExecutorTask and everything 
seemed to be OK. The way how I did it was very stupid (see the link to 
diff above) - I added run() into WrapperExecutorTask constructor what 
means that this thread was stopped in this call till the wrapped task 
was finished (ie Ant target execution finished).

The issue 25528 shown my mistake. If action is started from toolbar, 
the AntActionInstance is used and implementation of actionPerformed 
calls RequestProcessor.postRequest(...) what means that if action is 
started second time it waits in the queue till the previous one is 
finished. And in the scenario described in issue 25528 this will never 
happen - because second execution of action kills the previous one.

Back to my solution:
* the WrapperExecutorTask should be started by posting it to RP, 
otherwise it is blocking caller till the Ant target execution is 
finished
* (optionally) the AntActionInstance should use 
RequestProcessor.getDefault().post(...) which would also solve this 
problem, because second action would be started in separate thread


As for the importance of this issue:

I agree that it is serious problem, but I doubt many users are 
affected. The only case when this happens is when Ant script is 
executed from menu and the script never stops. I think it is not very 
typical.

I also think there will be 3.4.1 release soon and so I propose to 
leave this fix for 3.4.1. If you (Jesse and Silvio and others) still 
would like to see it fixed, I propose to revert the fix done in this 
issue (this change is simpler and could get approval after RC1). The 
revert will fix issue 25528, and this one can be fixed properly later 
for 3.4.1.

Waiting for your opinions/preferences.
Comment 19 Rich Unger 2002-07-23 18:28:09 UTC
Do I get a vote? :-)
Comment 20 David Konecny 2002-07-24 12:27:31 UTC
Fixed in file:

Checking in src/org/apache/tools/ant/module/run/TargetExecutor.java;
new revision: 1.23; previous revision: 1.22

This is good candiate for 3.4.1 (if there will be any). 
Comment 22 David Konecny 2002-11-29 19:00:32 UTC
Integrated into NB341. Nobody explicitly mentioned that this fix is
approved, but you did not protest against it either. So it is there.
Comment 23 Jan Becicka 2003-10-03 13:01:17 UTC
Closed