Re: [compress] Dealing with uncaught RuntimeExceptions (again)
Le dim. 27 juin 2021 à 21:15, Stefan Bodewig a écrit : > > On 2021-06-27, Gilles Sadowski wrote: > > > Hi. > > >> [...] > > >> it seemed Gilles was opposed to this idea > > > Rather (IIRC) my last comment was that it was your choice as to > > what the API should look like. > > Sorry, I didn't mean to misrepresent your POV. You didn't misrepresent my opinion; just that it doesn't count if the requirement is to not depart from what was done in the past in [Compress]. > > > My opinion on the matter was along Gary's lines (which is J. Bloch's > > rationale provided in "Effective Java"). > > Indeed I personally would indeed *not* pick option 1 because it puts > > the onus on the Commons library whereas input that does not comply > > with preconditions (i.e. a supported format) should unsurprisingly > > throw an IAE. > > In which case we need to catch all the other RuntimeExceptions and turn > them into IAEs, right? :-) Well, yes, if the code has detected illegal input! But, again, if prefer to throw an "IOException"... There is no arguing about taste and/or backward compatibility/ > Some if we want to throws any other specific RuntimeException following > Matt's suggestion. > > We are already throwing checked IOExceptions for invalid archives in > many many cases today. Our users expect us to do so for all invalid > archives - well some of them. Then fine (so to speak). :-} > As I said, we can as well document that each method could throw > arbitrary RuntimeExceptions, but I don't believe we can list the kinds > of RuntimeExceptions exhaustively Why not? Listing all runtime exceptions is considered part of good documentation. > - if we knew which exceptions can be > thrown, then we could as well check the error conditions ourselves > beforehand. I don't follow... Regards, Gilles > > Stefan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
Le dim. 27 juin 2021 à 19:05, Torsten Curdt a écrit : >> [...] > > > I'd argue that signaling this problem should be a checked exception. > > > IMO this provides a clearer contract to the user. > > > > It doesn't. The user would have a false sense of security believing so. > > > > I guess I disagree there - and so seem the authors of the JDK. At the time of Java 1 surely so. Today, you shouldn't bet on it. > But that's fine - I just wanted to give my 2 cents :) > I don't have enough stake anymore to keep this discussion going. This discussion happened some 15 (?) years ago for Commons Math. All were removed because they were just PITA (i.e. no believer in checked exceptions could demonstrate the slightest usefulness). I've read somewhere that Java was the only language having this duality. Right against everyone else? > > cheers, > Torsten - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
On 2021-06-27, Gilles Sadowski wrote: > Hi. >> [...] >> it seemed Gilles was opposed to this idea > Rather (IIRC) my last comment was that it was your choice as to > what the API should look like. Sorry, I didn't mean to misrepresent your POV. > My opinion on the matter was along Gary's lines (which is J. Bloch's > rationale provided in "Effective Java"). > Indeed I personally would indeed *not* pick option 1 because it puts > the onus on the Commons library whereas input that does not comply > with preconditions (i.e. a supported format) should unsurprisingly > throw an IAE. In which case we need to catch all the other RuntimeExceptions and turn them into IAEs, right? :-) Some if we want to throws any other specific RuntimeException following Matt's suggestion. We are already throwing checked IOExceptions for invalid archives in many many cases today. Our users expect us to do so for all invalid archives - well some of them. As I said, we can as well document that each method could throw arbitrary RuntimeExceptions, but I don't believe we can list the kinds of RuntimeExceptions exhaustively - if we knew which exceptions can be thrown, then we could as well check the error conditions ourselves beforehand. Stefan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
On 2021-06-27, Gary Gregory wrote:
> Catching all unchecked exceptions (UE) and rethrowing as checked exceptions
> (CE) feels like both a horror show and an exercise in futility, especially
> in order to appease some tool that complains today of one thing which may
> complain differently tomorrow, I really don't like that idea on paper.
Independent of the nonsense JFrog does our users have repeatedly tols us
they expect Compress to throw an IOException rather than a
RuntimeException for broken archives.
https://issues.apache.org/jira/browse/COMPRESS-169 fixed in Compress 1.4
ten years ago probably is the first one and it is easy to find many more
of this type (COMPRESS-424, COMPRESS-490, COMPRESS-131, COMPRESS-219
... here I stopped grepping through our changelog).
> Let's keep in mind that a CE means one can catch it and try to do something
> about it which is definitely not the case for a corrupted archive. I mean,
> you can download it again, sure, and end up where you started.
This is not quite true. You can not recover from an EOFException
either. There are lots of cases where we already throw IOExceptions for
irrecoverably corrupt archives today.
> IOW, let's do what we think is best, not what some tool some company wants
> to sell ("our tool is hard core and found 5 gazillion bugs in the open
> source echo system")
+1 can't argue with selecting the option we consider best for our users.
Stefan
-
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
Perhaps the key point here is throwing a more specific exception than RuntimeException? Even if it's a subclass of it. Adding the javadocs for which exceptions are allowed to be thrown might be sufficient to cover the DoS attacks. On Sun, Jun 27, 2021 at 12:05 PM Torsten Curdt wrote: > > > > > > Based on that premise we could also just forget about checked exceptions > > > altogether. > > > > As Gary said (as Joshua Bloch said, as many people said), checked > > exceptions are for recoverable errors. > > > > Maybe it boils down to the definition of "recoverable". > > > > Parsing an archive I personally don't see in that realm. > > > > Even if the archive is just garbage (or any non-supported file format)? > > > > Especially then. Similar to > > > https://docs.oracle.com/javase/7/docs/api/java/net/MalformedURLException.html > > > Comparing all the subclasses of > > https://docs.oracle.com/javase/7/docs/api/java/lang/RuntimeException.html > > vs > > https://docs.oracle.com/javase/7/docs/api/java/io/IOException.html > > I do see this case fit naturally as a subclass of IOException. > > > > > > I'd argue that signaling this problem should be a checked exception. > > > IMO this provides a clearer contract to the user. > > > > It doesn't. The user would have a false sense of security believing so. > > > > I guess I disagree there - and so seem the authors of the JDK. > But that's fine - I just wanted to give my 2 cents :) > I don't have enough stake anymore to keep this discussion going. > > cheers, > Torsten - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
> > > Based on that premise we could also just forget about checked exceptions > > altogether. > > As Gary said (as Joshua Bloch said, as many people said), checked > exceptions are for recoverable errors. > Maybe it boils down to the definition of "recoverable". > Parsing an archive I personally don't see in that realm. > > Even if the archive is just garbage (or any non-supported file format)? > Especially then. Similar to https://docs.oracle.com/javase/7/docs/api/java/net/MalformedURLException.html Comparing all the subclasses of https://docs.oracle.com/javase/7/docs/api/java/lang/RuntimeException.html vs https://docs.oracle.com/javase/7/docs/api/java/io/IOException.html I do see this case fit naturally as a subclass of IOException. > > I'd argue that signaling this problem should be a checked exception. > > IMO this provides a clearer contract to the user. > > It doesn't. The user would have a false sense of security believing so. > I guess I disagree there - and so seem the authors of the JDK. But that's fine - I just wanted to give my 2 cents :) I don't have enough stake anymore to keep this discussion going. cheers, Torsten
Re: [commons-pool] branch master updated: [POOL-395] Improve exception thrown in GenericObjectPool.borrowObject when pool is exhausted. Added BaseGenericObjectPool.setMessagesStatistics(boolean).
I will update Javadoc and add inline comments.
Gary
On Sun, Jun 27, 2021, 11:49 Phil Steitz wrote:
> It's hard to tell what the actual change is below with all of the
> formatting / cosmetic changes mixed it, but AFAICT there is no sync
> control to ensure consistency or currency of the stats reported. Some
> note in javadoc or somewhere should be added to make it clear that stats
> may not accurately reflect snapshot state at the time of the exception.
> If you want to get a guaranteed accurate snapshot, you would have to
> lock the pool when you take it, which we don't want to do.
>
> On 6/27/21 7:47 AM, ggreg...@apache.org wrote:
> > This is an automated email from the ASF dual-hosted git repository.
> >
> > ggregory pushed a commit to branch master
> > in repository https://gitbox.apache.org/repos/asf/commons-pool.git
> >
> >
> > The following commit(s) were added to refs/heads/master by this push:
> > new 26b0829 [POOL-395] Improve exception thrown in
> GenericObjectPool.borrowObject when pool is exhausted. Added
> BaseGenericObjectPool.setMessagesStatistics(boolean).
> > 26b0829 is described below
> >
> > commit 26b0829fef39729df84adb1c01eb55c944aff6d7
> > Author: Gary Gregory
> > AuthorDate: Sun Jun 27 10:47:03 2021 -0400
> >
> > [POOL-395] Improve exception thrown in
> GenericObjectPool.borrowObject
> > when pool is exhausted. Added
> > BaseGenericObjectPool.setMessagesStatistics(boolean).
> >
> > - Pick up Checkstyle LineLength module from Commons IO where there
> was
> > none before.
> > - Message sentences start with a capital letter.
> > - Better parameter names.
> > - Javadoc.
> > - Inline comments.
> > ---
> > checkstyle.xml | 4 +
> > src/changes/changes.xml| 3 +
> > .../org/apache/commons/pool2/KeyedObjectPool.java | 4 +-
> > .../commons/pool2/KeyedPooledObjectFactory.java| 4 +-
> > .../java/org/apache/commons/pool2/ObjectPool.java | 4 +-
> > .../apache/commons/pool2/PooledObjectFactory.java | 4 +-
> > .../commons/pool2/impl/BaseGenericObjectPool.java | 114
> -
> > .../commons/pool2/impl/GenericKeyedObjectPool.java | 48 +
> > .../commons/pool2/impl/GenericObjectPool.java | 46 +
> > .../pool2/impl/SoftReferenceObjectPool.java| 4 +-
> > .../pool2/TestBasePoolableObjectFactory.java | 4 +-
> > .../pool2/impl/TestAbandonedKeyedObjectPool.java | 4 +-
> > .../pool2/impl/TestAbandonedObjectPool.java| 4 +-
> > .../pool2/impl/TestGenericKeyedObjectPool.java | 64 +++-
> > .../commons/pool2/impl/TestGenericObjectPool.java | 28 -
> > 15 files changed, 230 insertions(+), 109 deletions(-)
> >
> > diff --git a/checkstyle.xml b/checkstyle.xml
> > index dca770b..3f884e1 100644
> > --- a/checkstyle.xml
> > +++ b/checkstyle.xml
> > @@ -74,4 +74,8 @@
> >
> >
> >
> > +
> > +
> > +
> > +
> >
> > diff --git a/src/changes/changes.xml b/src/changes/changes.xml
> > index 40ef624..2368098 100644
> > --- a/src/changes/changes.xml
> > +++ b/src/changes/changes.xml
> > @@ -69,6 +69,9 @@ The type attribute can be
> add,update,fix,remove.
> >
> > Handle validation exceptions during eviction. #85.
> >
> > +
> > + Improve exception thrown in GenericObjectPool.borrowObject when
> pool is exhausted. Added
> BaseGenericObjectPool.setMessagesStatistics(boolean).
> > +
> >
> >
> > Fix [WARNING] Old version of checkstyle detected. Consider
> updating to >= v8.30. Update Checktyle to 8.43.
> > diff --git a/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java
> b/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java
> > index ab52908..cac55ac 100644
> > --- a/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java
> > +++ b/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java
> > @@ -289,12 +289,12 @@ public interface KeyedObjectPool extends
> Closeable {
> >*
> >* @param key the key used to obtain the object
> >* @param obj a {@link #borrowObject borrowed} instance to be
> returned.
> > - * @param mode destroy activation context provided to the factory
> > + * @param destroyMode destroy activation context provided to the
> factory
> >*
> >* @throws Exception if the instance cannot be invalidated
> >* @since 2.9.0
> >*/
> > -default void invalidateObject(final K key, final V obj, final
> DestroyMode mode) throws Exception {
> > +default void invalidateObject(final K key, final V obj, final
> DestroyMode destroyMode) throws Exception {
> > invalidateObject(key, obj);
> > }
> >
> > diff --git
> a/src/main/java/org/apache/commons/pool2/KeyedPooledObjectFactory.java
> b/src/main/java/org/apache/commons/pool2/KeyedPooledObjectFactory.java
> > index ac93114..a7f75a3 100644
> > ---
>
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
Checked exceptions are also used when the error isn’t a programmer error. >From an aesthetic perspective, I prefer the unchecked exceptions unless an API already established them. Subclassing IOException is fairly common for example. On Sun, Jun 27, 2021 at 10:37 Torsten Curdt wrote: > > Can you expand on that - I don't understand the horror or the futility. > > > > > The futility is because it is wrong for the caller to expect that no > > runtime exception can be thrown. > > > > Based on that premise we could also just forget about checked exceptions > altogether. > > I think the distinction is that runtime exceptions are more for unexpected > errors. > Parsing an archive I personally don't see in that realm. > > > > > It certainly is a bit of code smell - but only because there is no > quick > > > way to harden the implementations. > > > > Maybe I do not follow what you mean, but there is no way to "harden": > > if the input is garbage, it cannot be processed and _some_ exception > > will result. > > > > If there are checks in place, the implementation could throw a checked > exception. > Harden as in adding those checks. > > > But I rather have a clear and stable API and fix the the implementation > > > along the way than to leak the problems to the API consumer and just > > > document it. > > > > Signalling that a problem with the input was detected is not leaking > > since the problem was the API caller/consumer in the first place. > > [Fixing library bugs is not what is being discussed AFAICT.] > > > > I'd argue that signaling this problem should be a checked exception. > IMO this provides a clearer contract to the user. > > I guess it's all about managing expectations. > If I throw garbage at a parser I'd not expect a runtime exception but a > ParseException. > If it does throw a runtime exception instead it is leaking the problem as > invalid input is not something the caller can even check beforehand. >
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
Hi. Le dim. 27 juin 2021 à 17:37, Torsten Curdt a écrit : > > > Can you expand on that - I don't understand the horror or the futility. > > > > > The futility is because it is wrong for the caller to expect that no > > runtime exception can be thrown. > > > > Based on that premise we could also just forget about checked exceptions > altogether. As Gary said (as Joshua Bloch said, as many people said), checked exceptions are for recoverable errors. > I think the distinction is that runtime exceptions are more for unexpected > errors. Agreed. > Parsing an archive I personally don't see in that realm. Even if the archive is just garbage (or any non-supported file format)? > > > It certainly is a bit of code smell - but only because there is no quick > > > way to harden the implementations. > > > > Maybe I do not follow what you mean, but there is no way to "harden": > > if the input is garbage, it cannot be processed and _some_ exception > > will result. > > > > If there are checks in place, the implementation could throw a checked > exception. > Harden as in adding those checks. The question is the rationale for choosing checked vs unchecked exceptions. > > But I rather have a clear and stable API and fix the the implementation > > > along the way than to leak the problems to the API consumer and just > > > document it. > > > > Signalling that a problem with the input was detected is not leaking > > since the problem was the API caller/consumer in the first place. > > [Fixing library bugs is not what is being discussed AFAICT.] > > > > I'd argue that signaling this problem should be a checked exception. > IMO this provides a clearer contract to the user. It doesn't. The user would have a false sense of security believing so. > I guess it's all about managing expectations. > If I throw garbage at a parser I'd not expect a runtime exception but a > ParseException. That's acceptable if the rationale is to behave like the JDK does in circumstances deemed similar. [But I still think that the one-rule of "recoverable or not" is easier to follow consistently.] > If it does throw a runtime exception instead it is leaking the problem as > invalid input is not something the caller can even check beforehand. In this particular case, it's true that we are on the fence for that very reason. It is however no less true that the error is not recoverable from the POV of the Commons library. For example, if it _could_ be checked beforehand that the input is wrong, we should (IMO) throw an IAE; hence it is indeed inconsistent that depending on how/when the error is discovered, the exception would be different. But as said previously, it could boil down to a choice of API. Regards, Gilles - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [commons-pool] branch master updated: [POOL-395] Improve exception thrown in GenericObjectPool.borrowObject when pool is exhausted. Added BaseGenericObjectPool.setMessagesStatistics(boolean).
It's hard to tell what the actual change is below with all of the
formatting / cosmetic changes mixed it, but AFAICT there is no sync
control to ensure consistency or currency of the stats reported. Some
note in javadoc or somewhere should be added to make it clear that stats
may not accurately reflect snapshot state at the time of the exception.
If you want to get a guaranteed accurate snapshot, you would have to
lock the pool when you take it, which we don't want to do.
On 6/27/21 7:47 AM, ggreg...@apache.org wrote:
This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-pool.git
The following commit(s) were added to refs/heads/master by this push:
new 26b0829 [POOL-395] Improve exception thrown in
GenericObjectPool.borrowObject when pool is exhausted. Added
BaseGenericObjectPool.setMessagesStatistics(boolean).
26b0829 is described below
commit 26b0829fef39729df84adb1c01eb55c944aff6d7
Author: Gary Gregory
AuthorDate: Sun Jun 27 10:47:03 2021 -0400
[POOL-395] Improve exception thrown in GenericObjectPool.borrowObject
when pool is exhausted. Added
BaseGenericObjectPool.setMessagesStatistics(boolean).
- Pick up Checkstyle LineLength module from Commons IO where there was
none before.
- Message sentences start with a capital letter.
- Better parameter names.
- Javadoc.
- Inline comments.
---
checkstyle.xml | 4 +
src/changes/changes.xml| 3 +
.../org/apache/commons/pool2/KeyedObjectPool.java | 4 +-
.../commons/pool2/KeyedPooledObjectFactory.java| 4 +-
.../java/org/apache/commons/pool2/ObjectPool.java | 4 +-
.../apache/commons/pool2/PooledObjectFactory.java | 4 +-
.../commons/pool2/impl/BaseGenericObjectPool.java | 114 -
.../commons/pool2/impl/GenericKeyedObjectPool.java | 48 +
.../commons/pool2/impl/GenericObjectPool.java | 46 +
.../pool2/impl/SoftReferenceObjectPool.java| 4 +-
.../pool2/TestBasePoolableObjectFactory.java | 4 +-
.../pool2/impl/TestAbandonedKeyedObjectPool.java | 4 +-
.../pool2/impl/TestAbandonedObjectPool.java| 4 +-
.../pool2/impl/TestGenericKeyedObjectPool.java | 64 +++-
.../commons/pool2/impl/TestGenericObjectPool.java | 28 -
15 files changed, 230 insertions(+), 109 deletions(-)
diff --git a/checkstyle.xml b/checkstyle.xml
index dca770b..3f884e1 100644
--- a/checkstyle.xml
+++ b/checkstyle.xml
@@ -74,4 +74,8 @@
+
+
+
+
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 40ef624..2368098 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -69,6 +69,9 @@ The type attribute can be add,update,fix,remove.
Handle validation exceptions during eviction. #85.
+
+ Improve exception thrown in GenericObjectPool.borrowObject when pool is
exhausted. Added BaseGenericObjectPool.setMessagesStatistics(boolean).
+
Fix [WARNING] Old version of checkstyle detected. Consider updating to
>= v8.30. Update Checktyle to 8.43.
diff --git a/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java
b/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java
index ab52908..cac55ac 100644
--- a/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java
+++ b/src/main/java/org/apache/commons/pool2/KeyedObjectPool.java
@@ -289,12 +289,12 @@ public interface KeyedObjectPool extends Closeable {
*
* @param key the key used to obtain the object
* @param obj a {@link #borrowObject borrowed} instance to be returned.
- * @param mode destroy activation context provided to the factory
+ * @param destroyMode destroy activation context provided to the factory
*
* @throws Exception if the instance cannot be invalidated
* @since 2.9.0
*/
-default void invalidateObject(final K key, final V obj, final DestroyMode
mode) throws Exception {
+default void invalidateObject(final K key, final V obj, final DestroyMode
destroyMode) throws Exception {
invalidateObject(key, obj);
}
diff --git a/src/main/java/org/apache/commons/pool2/KeyedPooledObjectFactory.java b/src/main/java/org/apache/commons/pool2/KeyedPooledObjectFactory.java
index ac93114..a7f75a3 100644
--- a/src/main/java/org/apache/commons/pool2/KeyedPooledObjectFactory.java
+++ b/src/main/java/org/apache/commons/pool2/KeyedPooledObjectFactory.java
@@ -117,7 +117,7 @@ public interface KeyedPooledObjectFactory {
*
* @param key the key used when selecting the instance
* @param p a {@code PooledObject} wrapping the instance to be destroyed
- * @param mode DestroyMode providing context to the factory
+ * @param destroyMode DestroyMode providing context to the
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
> Can you expand on that - I don't understand the horror or the futility. > > The futility is because it is wrong for the caller to expect that no > runtime exception can be thrown. > Based on that premise we could also just forget about checked exceptions altogether. I think the distinction is that runtime exceptions are more for unexpected errors. Parsing an archive I personally don't see in that realm. > > It certainly is a bit of code smell - but only because there is no quick > > way to harden the implementations. > > Maybe I do not follow what you mean, but there is no way to "harden": > if the input is garbage, it cannot be processed and _some_ exception > will result. > If there are checks in place, the implementation could throw a checked exception. Harden as in adding those checks. > But I rather have a clear and stable API and fix the the implementation > > along the way than to leak the problems to the API consumer and just > > document it. > > Signalling that a problem with the input was detected is not leaking > since the problem was the API caller/consumer in the first place. > [Fixing library bugs is not what is being discussed AFAICT.] > I'd argue that signaling this problem should be a checked exception. IMO this provides a clearer contract to the user. I guess it's all about managing expectations. If I throw garbage at a parser I'd not expect a runtime exception but a ParseException. If it does throw a runtime exception instead it is leaking the problem as invalid input is not something the caller can even check beforehand.
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
On Sun, 27 Jun 2021 at 15:01, sebb wrote: > > On Sun, 27 Jun 2021 at 14:06, Torsten Curdt wrote: > > > > > feels like both a horror show and an exercise in futility > > > > > > Can you expand on that - I don't understand the horror or the futility. > > > > It certainly is a bit of code smell - but only because there is no quick > > way to harden the implementations. > > > > But I rather have a clear and stable API and fix the the implementation > > along the way than to leak the problems to the API consumer and just > > document it. > > > > For me it's not about "appease some tool" it's just that the tool has > > revealed a problem and we have to decide how to deal with this in the > > future. > > Agreed. > At least some of the exceptions could presumably be avoided by > increased validation during processing. Sorry, pushed send by mistake. I meant to add: Wrapping the UE in an IOError or similar allows the code to provide more context to the user as to what was being done when the error occurred, and thus help debug the issue. > > cheers, > > Torsten - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
Hi. > [...] > it seemed Gilles was opposed to this idea Rather (IIRC) my last comment was that it was your choice as to what the API should look like. My opinion on the matter was along Gary's lines (which is J. Bloch's rationale provided in "Effective Java"). Indeed I personally would indeed *not* pick option 1 because it puts the onus on the Commons library whereas input that does not comply with preconditions (i.e. a supported format) should unsurprisingly throw an IAE. Of course, this is also a matter of taste e.g. if one wishes to behave similarly to the JDK for things like "FileNotFoundException"... > [...] Le dim. 27 juin 2021 à 15:06, Torsten Curdt a écrit : > > > feels like both a horror show and an exercise in futility > > > Can you expand on that - I don't understand the horror or the futility. The futility is because it is wrong for the caller to expect that no runtime exception can be thrown. > It certainly is a bit of code smell - but only because there is no quick > way to harden the implementations. Maybe I do not follow what you mean, but there is no way to "harden": if the input is garbage, it cannot be processed and _some_ exception will result. > But I rather have a clear and stable API and fix the the implementation > along the way than to leak the problems to the API consumer and just > document it. Signalling that a problem with the input was detected is not leaking since the problem was the API caller/consumer in the first place. [Fixing library bugs is not what is being discussed AFAICT.] Regards, Gilles > > For me it's not about "appease some tool" it's just that the tool has > revealed a problem and we have to decide how to deal with this in the > future. > > cheers, > Torsten - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
On Sun, 27 Jun 2021 at 14:06, Torsten Curdt wrote: > > > feels like both a horror show and an exercise in futility > > > Can you expand on that - I don't understand the horror or the futility. > > It certainly is a bit of code smell - but only because there is no quick > way to harden the implementations. > > But I rather have a clear and stable API and fix the the implementation > along the way than to leak the problems to the API consumer and just > document it. > > For me it's not about "appease some tool" it's just that the tool has > revealed a problem and we have to decide how to deal with this in the > future. Agreed. At least some of the exceptions could presumably be avoided by increased validation during processing. > cheers, > Torsten - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
> feels like both a horror show and an exercise in futility Can you expand on that - I don't understand the horror or the futility. It certainly is a bit of code smell - but only because there is no quick way to harden the implementations. But I rather have a clear and stable API and fix the the implementation along the way than to leak the problems to the API consumer and just document it. For me it's not about "appease some tool" it's just that the tool has revealed a problem and we have to decide how to deal with this in the future. cheers, Torsten
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
Catching all unchecked exceptions (UE) and rethrowing as checked exceptions
(CE) feels like both a horror show and an exercise in futility, especially
in order to appease some tool that complains today of one thing which may
complain differently tomorrow, I really don't like that idea on paper.
Let's keep in mind that a CE means one can catch it and try to do something
about it which is definitely not the case for a corrupted archive. I mean,
you can download it again, sure, and end up where you started.
The big picture for me is avoiding infinite loops and decompression bombs.
An defensive app will catch Exception anyway to catch IOException, NPE,
IAE, and ISE. We can document these where we know they can take place. We
can throw IOE, IAE or ISE to better document problems like bad array
indices.
IOW, let's do what we think is best, not what some tool some company wants
to sell ("our tool is hard core and found 5 gazillion bugs in the open
source echo system")
Gary
On Sun, Jun 27, 2021, 07:03 Stefan Bodewig wrote:
> Hi
>
> I'd like to get closure on which approach we want to take.
>
> When we read a broken archive it may trigger arbitrary RuntimeExceptions
> because we are not explicitly checking for each and every sizuation
> where a bounds check could fail, a negative size is sent to a classlib
> method that then throws an IllegalArgumentException or whatnot (even a
> NullPointerException may escape us every now and then).
>
> Uncaught RuntimeExceptions are considered security issued by some tools
> because of a potential DoS attack. Historically we have never agreed
> with this point of view and I'm not suggesting to change that.
>
> Even though we may not know what is wrong, when the RuntimeException
> occurs, we do know the archive is broken and this is the reason for the
> exception.
>
> AFAICS there are two ways we can deal with it:
>
> (1) every method that reads from the archive declares it can throw
> arbitrary RuntimeExceptions as well. And we document that broken
> archives may cause RuntimeExceptions and that we never consider such
> a case a security issue.
>
> (2) we catch RuntimeExceptions at every method that reads from the
> archive and wrap them in a custom IOException, making sure such a
> case can never escape us.
>
> Personally I prefer (2) but can live with (1) - I've suggested something
> along the lines of (2) in [1] and it seemed Gilles was opposed to this
> idea (and Matt was torn).
>
> In [2] Bernd seemed to support (2).
>
> Are there any other opinions?
>
> Stefan
>
> [1]
> https://lists.apache.org/thread.html/r5d2427566dff4c7d293e8d48f9ac62b7958d19047f730836ce5b3c60%40%3Cdev.commons.apache.org%3E
>
> [2]
> https://lists.apache.org/thread.html/r3ce77eb9ab9429097ca57c48cb99b8be497ee5b69d419b52a6722616%40%3Cdev.commons.apache.org%3E
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
+1 for 2) In [Imaging] we do something similar, but it was easier because there were many methods that were already doing it (throwing ImageReadException or ImageWriteException). Bruno On Sunday, 27 June 2021, 11:03:31 pm NZST, Stefan Bodewig wrote: Hi I'd like to get closure on which approach we want to take. When we read a broken archive it may trigger arbitrary RuntimeExceptions because we are not explicitly checking for each and every sizuation where a bounds check could fail, a negative size is sent to a classlib method that then throws an IllegalArgumentException or whatnot (even a NullPointerException may escape us every now and then). Uncaught RuntimeExceptions are considered security issued by some tools because of a potential DoS attack. Historically we have never agreed with this point of view and I'm not suggesting to change that. Even though we may not know what is wrong, when the RuntimeException occurs, we do know the archive is broken and this is the reason for the exception. AFAICS there are two ways we can deal with it: (1) every method that reads from the archive declares it can throw arbitrary RuntimeExceptions as well. And we document that broken archives may cause RuntimeExceptions and that we never consider such a case a security issue. (2) we catch RuntimeExceptions at every method that reads from the archive and wrap them in a custom IOException, making sure such a case can never escape us. Personally I prefer (2) but can live with (1) - I've suggested something along the lines of (2) in [1] and it seemed Gilles was opposed to this idea (and Matt was torn). In [2] Bernd seemed to support (2). Are there any other opinions? Stefan [1] https://lists.apache.org/thread.html/r5d2427566dff4c7d293e8d48f9ac62b7958d19047f730836ce5b3c60%40%3Cdev.commons.apache.org%3E [2] https://lists.apache.org/thread.html/r3ce77eb9ab9429097ca57c48cb99b8be497ee5b69d419b52a6722616%40%3Cdev.commons.apache.org%3E - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] Dealing with uncaught RuntimeExceptions (again)
I personally think that we should look at it from the point of view of the API consumer. What would they want to happen in case of an error reading an archive? IMO an IOException. How we get to that exception on the implementation side is another matter. If we could check all conditions and throw "legitimate" IO exceptions - it would be perfect. Since that's unfortunately not feasible for the time being, wrapping a runtime exceptions sounds like the most pragmatic and future proof approach to me. So I am in favor of 2) cheers, Torsten
[compress] Dealing with uncaught RuntimeExceptions (again)
Hi I'd like to get closure on which approach we want to take. When we read a broken archive it may trigger arbitrary RuntimeExceptions because we are not explicitly checking for each and every sizuation where a bounds check could fail, a negative size is sent to a classlib method that then throws an IllegalArgumentException or whatnot (even a NullPointerException may escape us every now and then). Uncaught RuntimeExceptions are considered security issued by some tools because of a potential DoS attack. Historically we have never agreed with this point of view and I'm not suggesting to change that. Even though we may not know what is wrong, when the RuntimeException occurs, we do know the archive is broken and this is the reason for the exception. AFAICS there are two ways we can deal with it: (1) every method that reads from the archive declares it can throw arbitrary RuntimeExceptions as well. And we document that broken archives may cause RuntimeExceptions and that we never consider such a case a security issue. (2) we catch RuntimeExceptions at every method that reads from the archive and wrap them in a custom IOException, making sure such a case can never escape us. Personally I prefer (2) but can live with (1) - I've suggested something along the lines of (2) in [1] and it seemed Gilles was opposed to this idea (and Matt was torn). In [2] Bernd seemed to support (2). Are there any other opinions? Stefan [1] https://lists.apache.org/thread.html/r5d2427566dff4c7d293e8d48f9ac62b7958d19047f730836ce5b3c60%40%3Cdev.commons.apache.org%3E [2] https://lists.apache.org/thread.html/r3ce77eb9ab9429097ca57c48cb99b8be497ee5b69d419b52a6722616%40%3Cdev.commons.apache.org%3E - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
