Re: new RuntimeException(Throwable) vs SneakyThrows
Julian> Do you propose to ban ’throw new RuntimeException(e)’ in all code or just in tests? I'm 100.42% sure we should ban new RuntimeException(e) and all the other cases like "new Throwable(e)" in test code. pull/1042 touches test code only, so it is more-or-less simple to reason about. That is why I've put rethrow into TestUtil. Julian> in all code That's the question. I'm inclined (up to 99.42%) to ban it in non-test code as well as the benefits greatly overweight drawbacks. Julian> What should people do in non-test code? My idea is to add Calcite-specfic CalciteThrowables#rethrow and use it all over the place (naming and placement is still to be defined). Julian>Use guava’s ’throw Throwables.propagate’ That should never be used. Even Guava deprecated it. We need sneaky-throws, and Guava hasn't that. Julian>I am not inclined to adopt solutions that would make our overall code quality worse. The quality of the codebase won't suffer, however we would get way better exceptions should something fail in runtime. PS. Fun fact: I've seen multiple production outages caused by OutOfMemory when exception class was trying to put full stacktrace into the message. The resulting message took 500+ megabytes even though the initial exception was just 5-10 kilobytes. Vladimir
Re: new RuntimeException(Throwable) vs SneakyThrows
Do you propose to ban ’throw new RuntimeException(e)’ in all code or just in
tests? The alternative, ’throw rethrow(e)' is only available in test code
(because rethrow lives in TestUtil).
What should people do in non-test code? Continue to ’throw new
RuntimeException’? Use guava’s ’throw Throwables.propagate’? We need clarity.
I don’t see a huge problem here — test error stacks that are a bit hard to read
— so I am not inclined to adopt solutions that would make our overall code
quality worse.
Julian
> On Feb 14, 2019, at 10:26 AM, Stamatis Zampetakis wrote:
>
> Thanks for starting this discussion Vladimir.
>
> Although sneaky throws pattern is a rather hacky way of propagating a
> checked exception it does improve its readability. I like it. Till now I
> never noticed how verbose was a checked exception wrapped in a runtime
> exception.
>
> I agree that the pattern new RuntimeException(e) is a rather bad idea but
> if we forbid this we may need to forbid also new IllegalStateException(e),
> new IllegalArgumentException(e) etc., otherwise developers may turned into
> these as an alternative. I don't know how far we want push this given that
> the new RuntimeException(e) is the number one tool used by developers for
> propagating checked exceptions (even IDEs autocomplete catch blocks adding
> such code).
>
> Overall, I am +1 on this.
>
> Best,
> Stamatis
>
> On Thu, Feb 14, 2019, 8:44 AM Vladimir Sitnikov wrote:
>
>> Julian> then there is a real chance that people will instead write
>> Julian> System.out.println(e);
>>
>> Isn't System.out banned already?
>> Of course we would just ban it and that's it.
>>
>> Julian>new RuntimeException(e), which I don’t think is that bad
>>
>> It produces garbage in the exception stacktraces (extra meaningless frames,
>> and a duplicate message which is confusing).
>> Human-readable exception messages are more important than human-readable
>> commit messages IMO.
>>
>>
>> Well, in fact, there are multiple solutions.
>> Sneaky-throws is a no-brainer that produces even better results than new
>> RuntimeException(e).
>> So I think behind the lines of
>>
>> @defaultMessage RuntimeException(java.lang.Throwable) duplicates
>> message, so consider use of CalciteResource#rethrow or use an explicit
>> message
>> java.lang.RuntimeException#(java.lang.Throwable)
>>
>>
>> 1) Sneaky-throws. It is as simple as catch(Throwable t) { throw rethrow(t);
>> }
>> A single catch fits all, and it can even automatically unwrap
>> InvocationTargetException.
>> The downsides might include:
>> 1.a) "unexpected" SQLException as a result of a "nothrows" method. Even
>> though it might be confusing, I think developer more or less expects
>> SQLException (or alike) when dealing with SQL.
>> 1.b) "missing vital debugging information". For instance, Avatica has
>> "execute prepared statement" block (see
>>
>> https://github.com/apache/calcite-avatica/blob/80ccd2007c5d34748852b019f1d1d10ebc94392a/core/src/main/java/org/apache/calcite/avatica/AvaticaConnection.java#L555-L560
>> ), and it would be great if it printed bind parameters in case of a
>> failure. Current code just adds "Error while executing a prepared
>> statement" message that does not really help.
>>
>> 2) Wrap with RuntimeException("", e)
>> The only "good" thing is the approach is compatible with Java 1.4 way of
>> throwing exceptions.
>> The downsides are:
>> 2.a) It is quite verbose as it requires multiple catch blocks that often
>> repeat each other.
>> 2.b) "missing vital debugging information"==1.b
>> 2.c) Stacktrace is expanded (it includes RuntimeException+frames), and it
>> adds absolutely nothing meaningful
>>
>> 3) Wrap with RuntimeException(meaninfulMessageThatAddsInfo, e)
>> This is more or less OK provided the message does not duplicate
>> e.getMessage()
>> At the end of the day, e.getMessage() would be printed anyway, so why
>> duplicate it?
>>
>> 4) Use checked exceptions or
>> even org.apache.calcite.runtime.CalciteResource. That could even support
>> localization, however it might require non-trivial efforts to use properly.
>> For instance use CalciteResources in test code does not pay off. On top of
>> that, checked exceptions are not compatible with lambdas.
>>
>> Vladimir
>>
Re: new RuntimeException(Throwable) vs SneakyThrows
Thanks for starting this discussion Vladimir.
Although sneaky throws pattern is a rather hacky way of propagating a
checked exception it does improve its readability. I like it. Till now I
never noticed how verbose was a checked exception wrapped in a runtime
exception.
I agree that the pattern new RuntimeException(e) is a rather bad idea but
if we forbid this we may need to forbid also new IllegalStateException(e),
new IllegalArgumentException(e) etc., otherwise developers may turned into
these as an alternative. I don't know how far we want push this given that
the new RuntimeException(e) is the number one tool used by developers for
propagating checked exceptions (even IDEs autocomplete catch blocks adding
such code).
Overall, I am +1 on this.
Best,
Stamatis
On Thu, Feb 14, 2019, 8:44 AM Vladimir Sitnikov Julian> then there is a real chance that people will instead write
> Julian> System.out.println(e);
>
> Isn't System.out banned already?
> Of course we would just ban it and that's it.
>
> Julian>new RuntimeException(e), which I don’t think is that bad
>
> It produces garbage in the exception stacktraces (extra meaningless frames,
> and a duplicate message which is confusing).
> Human-readable exception messages are more important than human-readable
> commit messages IMO.
>
>
> Well, in fact, there are multiple solutions.
> Sneaky-throws is a no-brainer that produces even better results than new
> RuntimeException(e).
> So I think behind the lines of
>
> @defaultMessage RuntimeException(java.lang.Throwable) duplicates
> message, so consider use of CalciteResource#rethrow or use an explicit
> message
> java.lang.RuntimeException#(java.lang.Throwable)
>
>
> 1) Sneaky-throws. It is as simple as catch(Throwable t) { throw rethrow(t);
> }
> A single catch fits all, and it can even automatically unwrap
> InvocationTargetException.
> The downsides might include:
> 1.a) "unexpected" SQLException as a result of a "nothrows" method. Even
> though it might be confusing, I think developer more or less expects
> SQLException (or alike) when dealing with SQL.
> 1.b) "missing vital debugging information". For instance, Avatica has
> "execute prepared statement" block (see
>
> https://github.com/apache/calcite-avatica/blob/80ccd2007c5d34748852b019f1d1d10ebc94392a/core/src/main/java/org/apache/calcite/avatica/AvaticaConnection.java#L555-L560
> ), and it would be great if it printed bind parameters in case of a
> failure. Current code just adds "Error while executing a prepared
> statement" message that does not really help.
>
> 2) Wrap with RuntimeException("", e)
> The only "good" thing is the approach is compatible with Java 1.4 way of
> throwing exceptions.
> The downsides are:
> 2.a) It is quite verbose as it requires multiple catch blocks that often
> repeat each other.
> 2.b) "missing vital debugging information"==1.b
> 2.c) Stacktrace is expanded (it includes RuntimeException+frames), and it
> adds absolutely nothing meaningful
>
> 3) Wrap with RuntimeException(meaninfulMessageThatAddsInfo, e)
> This is more or less OK provided the message does not duplicate
> e.getMessage()
> At the end of the day, e.getMessage() would be printed anyway, so why
> duplicate it?
>
> 4) Use checked exceptions or
> even org.apache.calcite.runtime.CalciteResource. That could even support
> localization, however it might require non-trivial efforts to use properly.
> For instance use CalciteResources in test code does not pay off. On top of
> that, checked exceptions are not compatible with lambdas.
>
> Vladimir
>
Re: new RuntimeException(Throwable) vs SneakyThrows
Julian> then there is a real chance that people will instead write
Julian> System.out.println(e);
Isn't System.out banned already?
Of course we would just ban it and that's it.
Julian>new RuntimeException(e), which I don’t think is that bad
It produces garbage in the exception stacktraces (extra meaningless frames,
and a duplicate message which is confusing).
Human-readable exception messages are more important than human-readable
commit messages IMO.
Well, in fact, there are multiple solutions.
Sneaky-throws is a no-brainer that produces even better results than new
RuntimeException(e).
So I think behind the lines of
@defaultMessage RuntimeException(java.lang.Throwable) duplicates
message, so consider use of CalciteResource#rethrow or use an explicit
message
java.lang.RuntimeException#(java.lang.Throwable)
1) Sneaky-throws. It is as simple as catch(Throwable t) { throw rethrow(t);
}
A single catch fits all, and it can even automatically unwrap
InvocationTargetException.
The downsides might include:
1.a) "unexpected" SQLException as a result of a "nothrows" method. Even
though it might be confusing, I think developer more or less expects
SQLException (or alike) when dealing with SQL.
1.b) "missing vital debugging information". For instance, Avatica has
"execute prepared statement" block (see
https://github.com/apache/calcite-avatica/blob/80ccd2007c5d34748852b019f1d1d10ebc94392a/core/src/main/java/org/apache/calcite/avatica/AvaticaConnection.java#L555-L560
), and it would be great if it printed bind parameters in case of a
failure. Current code just adds "Error while executing a prepared
statement" message that does not really help.
2) Wrap with RuntimeException("", e)
The only "good" thing is the approach is compatible with Java 1.4 way of
throwing exceptions.
The downsides are:
2.a) It is quite verbose as it requires multiple catch blocks that often
repeat each other.
2.b) "missing vital debugging information"==1.b
2.c) Stacktrace is expanded (it includes RuntimeException+frames), and it
adds absolutely nothing meaningful
3) Wrap with RuntimeException(meaninfulMessageThatAddsInfo, e)
This is more or less OK provided the message does not duplicate
e.getMessage()
At the end of the day, e.getMessage() would be printed anyway, so why
duplicate it?
4) Use checked exceptions or
even org.apache.calcite.runtime.CalciteResource. That could even support
localization, however it might require non-trivial efforts to use properly.
For instance use CalciteResources in test code does not pay off. On top of
that, checked exceptions are not compatible with lambdas.
Vladimir
Re: new RuntimeException(Throwable) vs SneakyThrows
If we ban
} catch (FooException e) {
throw new RuntimeException(e);
}
then there is a real chance that people will instead write
} catch (FooException e) {
System.out.println(e);
}
instead. So, let’s give them a good alternative, or just let them continue
writing "new RuntimeException”, which I don’t think is that bad.
Julian
> On Feb 13, 2019, at 1:33 PM, Enrico Olivelli wrote:
>
> Vladimir,
>
> Il giorno mer 13 feb 2019 alle ore 21:49 Vladimir Sitnikov
> mailto:sitnikov.vladi...@gmail.com>> ha scritto:
>>
>> Hi,
>>
>> I've recently discovered that new RuntimeException(Throwable) results in a
>> duplicated error messages.
>> In fact Throwable#(Throwable) just calls cause.getMessage() and uses
>> it as its own message.
>>
>> This makes exceptions harder to follow since the new RuntimeException(e) is
>> very often used just to overcome "checked exception" rule in Java.
>>
>> I've recently committed a deduplicate PR to Avatica:
>> https://github.com/apache/calcite-avatica/pull/84
>> As you can see, it removed "RuntimeException: ..." nonsense and it made
>> exceptions easier to understand. For instance, see
>> https://github.com/apache/calcite-avatica/pull/84/files#diff-d499001f31481c5f7151a81380c01a60L332
>>
>> I think we should avoid new RuntimeException(Throwable), and I wonder if we
>> should add that as a forbiddenapis rule.
>>
>> You can find a PR for *test* code:
>> https://github.com/apache/calcite/pull/1042
>>
>> I'm not sure if I should just go ahead and dodge new
>> RuntimeException(Throwable) from the main codebase as well.
>> Do you have any negative experience with sneaky-throws pattern?
>> I'm inclined to do so provided no-one objects within a week.
>
> I think sneaky-throws works well in your case, I mean to replace new
> RuntimeException(Throwable).
> The only problem is that users will find checked exception thrown in
> places where they aren't supposed to be thrown,
> anyway using RuntimeException is weird as well.
>
> So +1 from me
>
> just my 2 cents
> Enrico
>
>>
>> Vladimir
Re: new RuntimeException(Throwable) vs SneakyThrows
Vladimir, Il giorno mer 13 feb 2019 alle ore 21:49 Vladimir Sitnikov ha scritto: > > Hi, > > I've recently discovered that new RuntimeException(Throwable) results in a > duplicated error messages. > In fact Throwable#(Throwable) just calls cause.getMessage() and uses > it as its own message. > > This makes exceptions harder to follow since the new RuntimeException(e) is > very often used just to overcome "checked exception" rule in Java. > > I've recently committed a deduplicate PR to Avatica: > https://github.com/apache/calcite-avatica/pull/84 > As you can see, it removed "RuntimeException: ..." nonsense and it made > exceptions easier to understand. For instance, see > https://github.com/apache/calcite-avatica/pull/84/files#diff-d499001f31481c5f7151a81380c01a60L332 > > I think we should avoid new RuntimeException(Throwable), and I wonder if we > should add that as a forbiddenapis rule. > > You can find a PR for *test* code: > https://github.com/apache/calcite/pull/1042 > > I'm not sure if I should just go ahead and dodge new > RuntimeException(Throwable) from the main codebase as well. > Do you have any negative experience with sneaky-throws pattern? > I'm inclined to do so provided no-one objects within a week. I think sneaky-throws works well in your case, I mean to replace new RuntimeException(Throwable). The only problem is that users will find checked exception thrown in places where they aren't supposed to be thrown, anyway using RuntimeException is weird as well. So +1 from me just my 2 cents Enrico > > Vladimir
