sebb wrote:
> On 17/03/2010, Mark Thomas <[email protected]> wrote:
>> On 17/03/2010 22:04, sebb wrote:
>>
>>> On 17/03/2010, Mark Thomas<[email protected]> 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:
>> [email protected]
>>>> For additional commands, e-mail: [email protected]
>>>>
>>>>
>>>>
>>>
>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail:
>> [email protected]
>>> For additional commands, e-mail: [email protected]
>>>
>>>
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [email protected]
>> For additional commands, e-mail: [email protected]
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
