Bug 50552 - Ant Tasks give a null pointer exception when an error occurs, masking true error
Summary: Ant Tasks give a null pointer exception when an error occurs, masking true error
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Catalina (show other bugs)
Version: unspecified
Hardware: All All
: P2 normal (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-06 12:39 UTC by chris.rimmer
Modified: 2011-01-08 17:23 UTC (History)
0 users



Attachments
Fix to abstract catalina task class to set up redirector correctly (544 bytes, application/octet-stream)
2011-01-06 12:39 UTC, chris.rimmer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description chris.rimmer 2011-01-06 12:39:49 UTC
Created attachment 26463 [details]
Fix to abstract catalina task class to set up redirector correctly

Using the Ant List Task with outputproperty set causes a null pointer exception in the event of any error. For example, if tomcat is not running or the url given is incorrect. Since the bug is in a base class, this probably affects all the ant tasks.

The bug is due to the fact that the code to clean up the redirector is called in the finally block, but the equivalent code to set it up is not. This problem is also present in Tomcat 6.

The attached patch fixes the problem. With the patch in place, using the list task when tomcat is down gives "Connection refused" instead.
Comment 1 Christopher Schultz 2011-01-06 14:09:05 UTC
Confirmed in Tomcat 7 trunk:

  <target name="bug50552test">
    <taskdef name="tomcat-list" classname="org.apache.catalina.ant.ListTask"/>
    <tomcat-list url="broken" username="username" password="password" outputproperty="foo" />
  </target>

Results in the following output:

bug50552test:

BUILD FAILED
/home/cschultz/projects/diagnosis/build.xml:1434: java.lang.NullPointerException
        at org.apache.tools.ant.taskdefs.Redirector.complete(Redirector.java:906)
        at org.apache.catalina.ant.BaseRedirectorHelperTask.closeRedirector(BaseRedirectorHelperTask.java:267)
        at org.apache.catalina.ant.AbstractCatalinaTask.execute(AbstractCatalinaTask.java:272)
        at org.apache.catalina.ant.AbstractCatalinaTask.execute(AbstractCatalinaTask.java:149)
        at org.apache.catalina.ant.ListTask.execute(ListTask.java:51)
        at org.apache.tools.ant.UnknownElement.execute(UnknownElement.java:291)
        at sun.reflect.GeneratedMethodAccessor4.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
        at java.lang.reflect.Method.invoke(Method.java:597)
        at org.apache.tools.ant.dispatch.DispatchUtils.execute(DispatchUtils.java:106)
        at org.apache.tools.ant.Task.perform(Task.java:348)
        at org.apache.tools.ant.Target.execute(Target.java:390)
        at org.apache.tools.ant.Target.performTasks(Target.java:411)
        at org.apache.tools.ant.Project.executeSortedTargets(Project.java:1397)
        at org.apache.tools.ant.Project.executeTarget(Project.java:1366)
        at org.apache.tools.ant.helper.DefaultExecutor.executeTargets(DefaultExecutor.java:41)
        at org.apache.tools.ant.Project.executeTargets(Project.java:1249)
        at org.apache.tools.ant.Main.runBuild(Main.java:801)
        at org.apache.tools.ant.Main.startAnt(Main.java:218)
        at org.apache.tools.ant.launch.Launcher.run(Launcher.java:280)
        at org.apache.tools.ant.launch.Launcher.main(Launcher.java:109)
Comment 2 Christopher Schultz 2011-01-06 16:39:45 UTC
A couple of comments:

1. This patch looks like it will fix the problem, though there are other options (see below)
2. Isn't this a bug in ant because it doesn't do NULL checking?
3. Does output redirection /ever/ work?

AbstractCatalinaTask.execute /never/ opens the redirector (BaseRedirectorHelperTask.openRedirector), yet calls BaseRedirectorHelperTask.closeRedirector in the finally block, unconditionally.

I think this can be fixed in one of several ways:

1. Use Chris Rimmer's patch
2. Modify BaseRedirectorHelperTask.closeRedirector to ensure that the streams have been opened before calling Redirector.complete
3. Have ant add null checking to Redirector.complete

Finally, it probably makes sense for AbstractCatalinaTask.execute to open these redirection streams at some point, which it does not appear to be doing, hence my question #3 above.
Comment 3 Christopher Schultz 2011-01-06 16:41:15 UTC
FYI I can confirm that when outputProperty is set and there are no errors, the output property /does/ get the contents of the manager task's reply.
Comment 4 Christopher Schultz 2011-01-06 16:58:20 UTC
It looks like this works properly in the sunny-day scenario because BaseRedirectorHelperTask.handleOutput is typically called, which properly opens the redirector:

    protected void handleOutput(String output) {
        if (redirectOutput) {
            if (redirectOutPrintStream == null) {
                openRedirector();
            }
            redirectOutPrintStream.println(output);
            if (alwaysLog) {
                log(output, Project.MSG_INFO);
            }

In the error case, AbstractCatalinaTask.execute does this:

            if (isFailOnError()) {
                throw new BuildException(e);
            } else {
                handleErrorOutput(e.getMessage());
            }

The BuildException doesn't get caught by the currently-running exception handler (for java.lang.Exception) and then the finally block is called, which attempts to close the redirector, ultimately resulting in this NPE.

I believe this bug might be better solved by modifying BaseRedirectorHelperTask.closeRedirector in this way:

        try {
-            if (redirectOutput) {
+            if (redirectOutput && redirectOutPrintStream != null) {

                redirector.complete();
            }

This makes BaseRedirectorHelperTask.closeRedirector safer internally and doesn't rely on the client (AbstractCatalinaTask, in this case) to be extra careful about state.

Another option would be:

        try {
            if (redirectOutput) {
+              if (redirectOutPrintStream == null) {
+                openRedirector();
+              }

                redirector.complete();
            }

I'm open to suggestions, since I don't really know how all this stuff should work. I'm in favor of a solution where a class manages it's state as much as possible rather than relying on client code (even a subclass) to be careful about such things.
Comment 5 chris.rimmer 2011-01-07 04:40:28 UTC
Btw I would have written a test or tests to show that my patch fixes the problem, but there don't seem to be any tests for these ant tasks. Unless I'm looking in the wrong place?
Comment 6 Mark Thomas 2011-01-07 06:28:10 UTC
Fixed in 7.0.x for 7.0.6 using Chris S's first proposal.

Chris R - You are correct. There are no tests for this code.

I also proposed the fix for 6.0.x
Comment 7 Christopher Schultz 2011-01-07 10:35:59 UTC
(In reply to comment #6)
> Fixed in 7.0.x for 7.0.6 using Chris S's first proposal.

I hadn't taken enough time to determine if that fix (adding the null check, right?) would be appropriate, since other streams in the redirector may have been open, and a call to complete() might be in order. If you've checked them out, then I'm happy.
Comment 8 Konstantin Kolinko 2011-01-08 17:23:10 UTC
Applied the patch to 6.0 in r1056813 and it will be in 6.0.30.

(In reply to comment #7):
I think the patch is OK. While it there are several places where opening of streams is triggered, the opening operation itself is consistently performed via a BaseRedirectorHelperTask#openRedirector() call. The check that we are doing is effectively whether openRedirector() was ever called or not.