I have been switching positions too myself but now I think that catching
Throwable is fine in some cases. For example
try{
writer.open()
} catch(Throwable th){
writer.fail();
throw th;
} finally{
writer.close();
}
If we replace Throwable with Exception in this case, then we have a small
problem and that is writer.fail() will not be called in some cases. One of the
common uses of writer.fail() is to tell the writers in the pipeline not to do
any additional work in the close() call since the pipeline has failed and
additional work could produce further failures.
So I think such uses is acceptable but other than using it to enforce the
contract, we should not catch Throwable.
Another way to think about this is to say that Errors are always terminal and
that it is time to call the shutdown hook and that we don't care if
writer.fail() was not called in that case and we probably assume that
writer.close will encounter a few additional failures. If we choose to follow
that, then we should document this somewhere for reference and then stick to it
everywhere.
Personally, I prefer to catch Throwable in restricted places but I am okay with
going route 2 as well.
~Abdullah.
> On Apr 13, 2017, at 8:02 PM, Till Westmann <[email protected]> wrote:
>
> Hi,
>
> I just looked at Abdullah’s change [1] which adds more consistency around
> the behavior of our operators in error cases. In the change there are a
> number of SQ complaints about catching Throwable [2] and as I always
> wondered what the right approach on this is (changing my mind a few times),
> I’d like to discuss this here and capture the result to apply the outcome
> consistently across the system.
>
> Please reply to this with your thoughts on catching Throwable vs not
> catching Error.
>
> Thanks,
> Till
>
> [1] https://asterix-gerrit.ics.uci.edu/#/c/1618
> [2] https://asterix-sonar.ics.uci.edu/coding_rules#rule_key=squid%3AS1181
