You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Mark Thomas <ma...@apache.org> on 2010/03/17 21:53:24 UTC
[POOL] Test failures
One of the POOL test cases is failing -
TestSoftRefOutOfMemory.testOutOfMemoryError()
I can't see any recent code changes that may have caused this and I
think the recent removal of the testAll code is what has exposed this.
On the basis that always catching Throwable is bad, but there are many
cases POOL does need to catch, what do folks here think to the liberal
application of the following anywhere POOL catches Throwable?
Index: src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
===================================================================
--- src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
(revision 924253)
+++ src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
(working copy)
@@ -106,10 +106,18 @@
throw new Exception("ValidateObject failed");
}
} catch (Throwable t) {
+ if (t instanceof VirtualMachineError) {
+ // Always throw VM errors immediately
+ throw (VirtualMachineError) t;
+ }
try {
_factory.destroyObject(obj);
} catch (Throwable t2) {
- // swallowed
+ if (t2 instanceof VirtualMachineError) {
+ // Always throw VM errors immediately
+ throw (VirtualMachineError) t2;
+ }
+ // Otherwise swallowed
} finally {
obj = null;
}
This fixes the current test failures but really should be applied to all
places where Throwable is caught.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [POOL] Test failures
Posted by Phil Steitz <ph...@gmail.com>.
sebb wrote:
> On 17/03/2010, Mark Thomas <ma...@apache.org> wrote:
>> On 17/03/2010 22:04, sebb wrote:
>>
>>> On 17/03/2010, Mark Thomas<ma...@apache.org> wrote:
>>>
>>>> One of the POOL test cases is failing -
>>>> TestSoftRefOutOfMemory.testOutOfMemoryError()
>>>>
>>>> I can't see any recent code changes that may have caused this and I
>> think
>>>> the recent removal of the testAll code is what has exposed this. On the
>>>> basis that always catching Throwable is bad, but there are many cases
>> POOL
>>>> does need to catch, what do folks here think to the liberal application
>> of
>>>> the following anywhere POOL catches Throwable?
OK with your suggestion.
>>>>
>>> ThreadDeath should never be ignored either.
>>>
>> Yep.
+1
>>
>>
>>> Is it necessary to swallow all Throwables apart from VME?
>>>
>> That depends on your definition of necessary. I'd like to keep the list of
>> exceptions as small as possible.
>>
>> On reflection, I think I'll put this in a utility method so the list of
>> exceptions only has to be maintained in a single place. That will help keep
>> the code clean even if we end up with a long list of Throwables we don't
>> want to swallow.
>
> Sounds good.
+1
>
> Seems to me it would be useful to be able to record or log the
> swallowed Throwables.
>
> If the Throwable is caused by a user error, then swallowing it means
> that the error is likely to be much harder to diagnose and fix.
I agree with this. My only concern is users who are relying on
current behavior.
>
>>> Maybe should rethrow some other classes of Throwable as well.
>>>
>>> Is it known which Throwables Pool does need to catch?
>>>
>> No fixed list. I'd say as many as possible.
>>
>> Mark
>>
>>
>>
>>
>>>
>>>> Index:
>>>>
>> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
>> ===================================================================
>>>> ---
>>>>
>> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
>>>> (revision 924253)
>>>> +++
>>>>
>> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
>>>> (working copy)
>>>> @@ -106,10 +106,18 @@
>>>> throw new Exception("ValidateObject failed");
>>>> }
>>>> } catch (Throwable t) {
>>>> + if (t instanceof VirtualMachineError) {
>>>> + // Always throw VM errors immediately
>>>> + throw (VirtualMachineError) t;
>>>> + }
>>>> try {
>>>> _factory.destroyObject(obj);
>>>> } catch (Throwable t2) {
>>>> - // swallowed
>>>> + if (t2 instanceof VirtualMachineError) {
>>>> + // Always throw VM errors immediately
>>>> + throw (VirtualMachineError) t2;
>>>> + }
>>>> + // Otherwise swallowed
>>>> } finally {
>>>> obj = null;
>>>> }
>>>>
>>>> This fixes the current test failures but really should be applied to
>> all
>>>> places where Throwable is caught.
>>>>
>>>> Mark
>>>>
>>>>
>>>>
>>>>
>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail:
>> dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>>>
>>>>
>>>
>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail:
>> dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>>
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [POOL] Test failures
Posted by sebb <se...@gmail.com>.
On 17/03/2010, Mark Thomas <ma...@apache.org> wrote:
> On 17/03/2010 22:04, sebb wrote:
>
> > On 17/03/2010, Mark Thomas<ma...@apache.org> wrote:
> >
> > > One of the POOL test cases is failing -
> > > TestSoftRefOutOfMemory.testOutOfMemoryError()
> > >
> > > I can't see any recent code changes that may have caused this and I
> think
> > > the recent removal of the testAll code is what has exposed this. On the
> > > basis that always catching Throwable is bad, but there are many cases
> POOL
> > > does need to catch, what do folks here think to the liberal application
> of
> > > the following anywhere POOL catches Throwable?
> > >
> >
> > ThreadDeath should never be ignored either.
> >
> Yep.
>
>
> > Is it necessary to swallow all Throwables apart from VME?
> >
> That depends on your definition of necessary. I'd like to keep the list of
> exceptions as small as possible.
>
> On reflection, I think I'll put this in a utility method so the list of
> exceptions only has to be maintained in a single place. That will help keep
> the code clean even if we end up with a long list of Throwables we don't
> want to swallow.
Sounds good.
Seems to me it would be useful to be able to record or log the
swallowed Throwables.
If the Throwable is caused by a user error, then swallowing it means
that the error is likely to be much harder to diagnose and fix.
>
> > Maybe should rethrow some other classes of Throwable as well.
> >
> > Is it known which Throwables Pool does need to catch?
> >
> No fixed list. I'd say as many as possible.
>
> Mark
>
>
>
>
> >
> >
> > > Index:
> > >
> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
> > >
> ===================================================================
> > > ---
> > >
> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
> > > (revision 924253)
> > > +++
> > >
> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
> > > (working copy)
> > > @@ -106,10 +106,18 @@
> > > throw new Exception("ValidateObject failed");
> > > }
> > > } catch (Throwable t) {
> > > + if (t instanceof VirtualMachineError) {
> > > + // Always throw VM errors immediately
> > > + throw (VirtualMachineError) t;
> > > + }
> > > try {
> > > _factory.destroyObject(obj);
> > > } catch (Throwable t2) {
> > > - // swallowed
> > > + if (t2 instanceof VirtualMachineError) {
> > > + // Always throw VM errors immediately
> > > + throw (VirtualMachineError) t2;
> > > + }
> > > + // Otherwise swallowed
> > > } finally {
> > > obj = null;
> > > }
> > >
> > > This fixes the current test failures but really should be applied to
> all
> > > places where Throwable is caught.
> > >
> > > Mark
> > >
> > >
> > >
> > >
> ---------------------------------------------------------------------
> > > To unsubscribe, e-mail:
> dev-unsubscribe@commons.apache.org
> > > For additional commands, e-mail: dev-help@commons.apache.org
> > >
> > >
> > >
> >
> >
> ---------------------------------------------------------------------
> > To unsubscribe, e-mail:
> dev-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
> >
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [POOL] Test failures
Posted by Mark Thomas <ma...@apache.org>.
On 17/03/2010 22:04, sebb wrote:
> On 17/03/2010, Mark Thomas<ma...@apache.org> wrote:
>> One of the POOL test cases is failing -
>> TestSoftRefOutOfMemory.testOutOfMemoryError()
>>
>> I can't see any recent code changes that may have caused this and I think
>> the recent removal of the testAll code is what has exposed this. On the
>> basis that always catching Throwable is bad, but there are many cases POOL
>> does need to catch, what do folks here think to the liberal application of
>> the following anywhere POOL catches Throwable?
>
> ThreadDeath should never be ignored either.
Yep.
> Is it necessary to swallow all Throwables apart from VME?
That depends on your definition of necessary. I'd like to keep the list
of exceptions as small as possible.
On reflection, I think I'll put this in a utility method so the list of
exceptions only has to be maintained in a single place. That will help
keep the code clean even if we end up with a long list of Throwables we
don't want to swallow.
> Maybe should rethrow some other classes of Throwable as well.
>
> Is it known which Throwables Pool does need to catch?
No fixed list. I'd say as many as possible.
Mark
>
>> Index:
>> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
>> ===================================================================
>> ---
>> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
>> (revision 924253)
>> +++
>> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
>> (working copy)
>> @@ -106,10 +106,18 @@
>> throw new Exception("ValidateObject failed");
>> }
>> } catch (Throwable t) {
>> + if (t instanceof VirtualMachineError) {
>> + // Always throw VM errors immediately
>> + throw (VirtualMachineError) t;
>> + }
>> try {
>> _factory.destroyObject(obj);
>> } catch (Throwable t2) {
>> - // swallowed
>> + if (t2 instanceof VirtualMachineError) {
>> + // Always throw VM errors immediately
>> + throw (VirtualMachineError) t2;
>> + }
>> + // Otherwise swallowed
>> } finally {
>> obj = null;
>> }
>>
>> This fixes the current test failures but really should be applied to all
>> places where Throwable is caught.
>>
>> Mark
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [POOL] Test failures
Posted by sebb <se...@gmail.com>.
On 17/03/2010, Mark Thomas <ma...@apache.org> wrote:
> One of the POOL test cases is failing -
> TestSoftRefOutOfMemory.testOutOfMemoryError()
>
> I can't see any recent code changes that may have caused this and I think
> the recent removal of the testAll code is what has exposed this. On the
> basis that always catching Throwable is bad, but there are many cases POOL
> does need to catch, what do folks here think to the liberal application of
> the following anywhere POOL catches Throwable?
ThreadDeath should never be ignored either.
Is it necessary to swallow all Throwables apart from VME?
Maybe should rethrow some other classes of Throwable as well.
Is it known which Throwables Pool does need to catch?
> Index:
> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
> ===================================================================
> ---
> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
> (revision 924253)
> +++
> src/java/org/apache/commons/pool/impl/SoftReferenceObjectPool.java
> (working copy)
> @@ -106,10 +106,18 @@
> throw new Exception("ValidateObject failed");
> }
> } catch (Throwable t) {
> + if (t instanceof VirtualMachineError) {
> + // Always throw VM errors immediately
> + throw (VirtualMachineError) t;
> + }
> try {
> _factory.destroyObject(obj);
> } catch (Throwable t2) {
> - // swallowed
> + if (t2 instanceof VirtualMachineError) {
> + // Always throw VM errors immediately
> + throw (VirtualMachineError) t2;
> + }
> + // Otherwise swallowed
> } finally {
> obj = null;
> }
>
> This fixes the current test failures but really should be applied to all
> places where Throwable is caught.
>
> Mark
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org