Pivot
  1. Pivot
  2. PIVOT-769

A Task's TaskListener will not be called if a Throwable is thown when the task is executed

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.1
    • Component/s: core-util
    • Labels:
      None

      Description

      org.apache.pivot.util.concurrent.Task.ExecuteCallback.run() only catches and records Exceptions as 'faults'.

      try

      { result = execute(); }

      catch(Exception exception)

      { fault = exception; }

      If a java.lang.Error or other non-Exception Throwable is thrown while the task is executing, the thread will be killed off and neither of the TaskListener callback methods will be executed.
      Also, nothing will be logged to syserr, sysout or elsewhere meaning the Throwable is 'swallowed' without any notification.

        Activity

        Hide
        Sandro Martini added a comment -

        Hi Chris,
        I haven't looked at it, but I think that at least we can add a
        System.err.println(exception);
        and probably we can catch Throwable (as we already do in some critical places) ... if you want/have some time, do it.
        Then something other has to be added in your opinion ?

        Note: if you find other places where an Exception is 'swallowed' without any notification, write here so we can see what to do (I fixed some warnings on this with FindBugs) ...

        Bye

        Show
        Sandro Martini added a comment - Hi Chris, I haven't looked at it, but I think that at least we can add a System.err.println(exception); and probably we can catch Throwable (as we already do in some critical places) ... if you want/have some time, do it. Then something other has to be added in your opinion ? Note: if you find other places where an Exception is 'swallowed' without any notification, write here so we can see what to do (I fixed some warnings on this with FindBugs) ... Bye
        Hide
        Chris Bartlett added a comment -

        Writing something to System.err is the minimum we should do, in my opinion. At least there would be some evidence of the failure, even though it might not be seen.

        I think that catching Throwable and 'logging' it internally as a 'fault' would be better.
        Anyone else have any suggestions?

        Show
        Chris Bartlett added a comment - Writing something to System.err is the minimum we should do, in my opinion. At least there would be some evidence of the failure, even though it might not be seen. I think that catching Throwable and 'logging' it internally as a 'fault' would be better. Anyone else have any suggestions?
        Hide
        Greg Brown added a comment -

        Why not change the API such that fault is a Throwable instead of an Exception?

        Show
        Greg Brown added a comment - Why not change the API such that fault is a Throwable instead of an Exception?
        Hide
        Chris Bartlett added a comment -

        Yep, that is what I meant.

        Exceptions are currently caught and stored in a field named 'fault' of type Exception.
        Change it so that 'fault' is a Throwable, Throwables are caught and stored in 'fault', and the getter returns a Throwable.

        Show
        Chris Bartlett added a comment - Yep, that is what I meant. Exceptions are currently caught and stored in a field named 'fault' of type Exception. Change it so that 'fault' is a Throwable, Throwables are caught and stored in 'fault', and the getter returns a Throwable.
        Hide
        Greg Brown added a comment -

        I am confused, then. If fault is changed to a Throwable such that the caller can "catch" it directly, what value does logging to System.err offer?

        Show
        Greg Brown added a comment - I am confused, then. If fault is changed to a Throwable such that the caller can "catch" it directly, what value does logging to System.err offer?
        Hide
        Chris Bartlett added a comment -

        Currently, a thrown Throwable will be silently swallowed, so simlpy writing something to System.err would be better than nothing.

        Throwables could possibly be handled in the same way that Exceptions are currently handled.

        • The Throwable would be caught in the ExecuteCallback class
        • it would be stored in the 'fault' field
        • the TaskListener's executeFailed() method would be invoked
        • the Throwable that caused the failure would be available via the Task's getFault() method.
        Show
        Chris Bartlett added a comment - Currently, a thrown Throwable will be silently swallowed, so simlpy writing something to System.err would be better than nothing. Throwables could possibly be handled in the same way that Exceptions are currently handled. The Throwable would be caught in the ExecuteCallback class it would be stored in the 'fault' field the TaskListener's executeFailed() method would be invoked the Throwable that caused the failure would be available via the Task's getFault() method.
        Hide
        Chris Bartlett added a comment -

        Just to clarify - Writing something to System.err is better than nothing, but would not be required in addition to a more complete solution.

        Show
        Chris Bartlett added a comment - Just to clarify - Writing something to System.err is better than nothing, but would not be required in addition to a more complete solution.
        Hide
        Greg Brown added a comment -

        Ah, OK. It seems like the right solution is just to catch Throwable and make "fault" a Throwable, then.

        Show
        Greg Brown added a comment - Ah, OK. It seems like the right solution is just to catch Throwable and make "fault" a Throwable, then.
        Hide
        Chris Bartlett added a comment -

        Changed 'fault' field to Throwable

        Show
        Chris Bartlett added a comment - Changed 'fault' field to Throwable
        Hide
        Bill van Melle added a comment -

        Although this was the logical change to make, I hope that somebody is recording this somewhere as a prominent backward-incompatible change in the next release. Anybody who has written an executeFailed method and done something as innocuous as saying

        Exception fault = task.getFault();

        will have to edit their code for it to run under 2.0.1 (as I have had to do just now when I updated to the latest trunk).

        Show
        Bill van Melle added a comment - Although this was the logical change to make, I hope that somebody is recording this somewhere as a prominent backward-incompatible change in the next release. Anybody who has written an executeFailed method and done something as innocuous as saying Exception fault = task.getFault(); will have to edit their code for it to run under 2.0.1 (as I have had to do just now when I updated to the latest trunk).
        Hide
        Chris Bartlett added a comment -

        A valid point, and one that the release process does take care of as far as I am aware.
        The JIRA tickets that are resolved and included in each release are listed in the release notes.
        Pivot 2.0 release notes are available here http://pivot.apache.org/download.cgi#2.0

        While the Pivot API is evolving, changes are generally not made without reason, and without the opportunity for discussion.

        A bit of background - I discovered this issue after a previously working app of mine was behaving strangely due to some thread checking changes that had been made elsewhere. These caused Errors to be thrown which were being silently swallowed. (I think it was java.lang.ExceptionInInitializerError in this instance). Hopefully this will help others to catch similar problems with less effort.

        Show
        Chris Bartlett added a comment - A valid point, and one that the release process does take care of as far as I am aware. The JIRA tickets that are resolved and included in each release are listed in the release notes. Pivot 2.0 release notes are available here http://pivot.apache.org/download.cgi#2.0 While the Pivot API is evolving, changes are generally not made without reason, and without the opportunity for discussion. A bit of background - I discovered this issue after a previously working app of mine was behaving strangely due to some thread checking changes that had been made elsewhere. These caused Errors to be thrown which were being silently swallowed. (I think it was java.lang.ExceptionInInitializerError in this instance). Hopefully this will help others to catch similar problems with less effort.
        Hide
        Bill van Melle added a comment -

        I'm aware of the automatic generation of release notes, which is great. But as a consumer of release notes, and trying to answer the question "should I upgrade?", I would love there to be a section called something like "Incompatible changes". Somebody by hand created something like that for 2.0 (https://cwiki.apache.org/confluence/display/PIVOT/Major+Feature+Changes+Between+1.5.x+and+2.0). Most of that page discusses features, but you also learn important things like that WTKXSerializer was renamed and the Shape classes were removed. Changing Exception to Throwable in one API isn't nearly that big, but it would be nice if there were a way to somehow flag it for the benefit of code migrators, so that you wouldn't have to click on every single JIRA link to discover that one of them, only listed as "A Task's TaskListener will not be called if a Throwable is thown...", implies that you might have to edit your Task code.

        (Just to be clear, I'm not complaining at all that you're fixing this bug – it's a pretty nasty one, really. I had a similar one where I discovered that if a query times out, your executeFailed method never gets called on Mac OS. Speaking of which, I wonder why I never filed a ticket on that.)

        Show
        Bill van Melle added a comment - I'm aware of the automatic generation of release notes, which is great. But as a consumer of release notes, and trying to answer the question "should I upgrade?", I would love there to be a section called something like "Incompatible changes". Somebody by hand created something like that for 2.0 ( https://cwiki.apache.org/confluence/display/PIVOT/Major+Feature+Changes+Between+1.5.x+and+2.0 ). Most of that page discusses features, but you also learn important things like that WTKXSerializer was renamed and the Shape classes were removed. Changing Exception to Throwable in one API isn't nearly that big, but it would be nice if there were a way to somehow flag it for the benefit of code migrators, so that you wouldn't have to click on every single JIRA link to discover that one of them, only listed as "A Task's TaskListener will not be called if a Throwable is thown...", implies that you might have to edit your Task code. (Just to be clear, I'm not complaining at all that you're fixing this bug – it's a pretty nasty one, really. I had a similar one where I discovered that if a query times out, your executeFailed method never gets called on Mac OS. Speaking of which, I wonder why I never filed a ticket on that.)
        Hide
        Chris Bartlett added a comment - - edited

        I understand your concerns and share them too. API diffs can be useful, and could be incorporated into the release process (if they are not already) I think I used http://javadiff.sourceforge.net/ a while ago.

        I think this discussion is probably best continued on the user mailing list, if that is OK with you? I will start a thread there shortly. Hopefully more people will see it and chip in with ideas. That might lead to a JIRA ticket of its own requesting some specific things of the release process.

        (And please do file a JIRA ticket for that bug if you are able to replicate it)

        Show
        Chris Bartlett added a comment - - edited I understand your concerns and share them too. API diffs can be useful, and could be incorporated into the release process (if they are not already) I think I used http://javadiff.sourceforge.net/ a while ago. I think this discussion is probably best continued on the user mailing list, if that is OK with you? I will start a thread there shortly. Hopefully more people will see it and chip in with ideas. That might lead to a JIRA ticket of its own requesting some specific things of the release process. (And please do file a JIRA ticket for that bug if you are able to replicate it)
        Show
        Chris Bartlett added a comment - Link to the mailing list discussion on nabble http://apache-pivot-users.399431.n3.nabble.com/Suggestions-for-improving-the-Pivot-release-process-and-related-questions-td3208415.html Official Pivot mailing list info http://pivot.apache.org/lists.html

          People

          • Assignee:
            Chris Bartlett
            Reporter:
            Chris Bartlett
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development