You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by sebb <se...@gmail.com> on 2014/12/02 18:35:23 UTC
Re: svn commit: r1638340 - in /commons/proper/fileupload/trunk/src:
changes/changes.xml main/java/org/apache/commons/fileupload/FileUploadBase.java
On 11 November 2014 at 20:09, <tn...@apache.org> wrote:
> Author: tn
> Date: Tue Nov 11 20:09:32 2014
> New Revision: 1638340
>
> URL: http://svn.apache.org/r1638340
> Log:
> [FILEUPLOAD-242] Do not silently swallow all Throwables.
>
> Modified:
> commons/proper/fileupload/trunk/src/changes/changes.xml
> commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java
>
> Modified: commons/proper/fileupload/trunk/src/changes/changes.xml
> URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/changes/changes.xml?rev=1638340&r1=1638339&r2=1638340&view=diff
> ==============================================================================
> --- commons/proper/fileupload/trunk/src/changes/changes.xml (original)
> +++ commons/proper/fileupload/trunk/src/changes/changes.xml Tue Nov 11 20:09:32 2014
> @@ -44,6 +44,7 @@ The <action> type attribute can be add,u
>
> <body>
> <release version="1.4" date="TBA" description="TBA">
> + <action issue="FILEUPLOAD-242" dev="tn" type="fix">FileUploadBase - should not silently catch and ignore all Throwables</action>
> <action issue="FILEUPLOAD-257" dev="tn" type="fix">Fix Javadoc 1.8.0 errors</action>
> <action issue="FILEUPLOAD-234" dev="tn" type="fix">Fix section "Resource cleanup" of the user guide</action>
> <action issue="FILEUPLOAD-237" dev="tn" type="fix">Fix streaming example: use FileItem.getInputStream() instead of openStream()</action>
>
> Modified: commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java
> URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java?rev=1638340&r1=1638339&r2=1638340&view=diff
> ==============================================================================
> --- commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java (original)
> +++ commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java Tue Nov 11 20:09:32 2014
> @@ -366,8 +366,8 @@ public abstract class FileUploadBase {
> for (FileItem fileItem : items) {
> try {
> fileItem.delete();
> - } catch (Throwable e) {
> - // ignore it
> + } catch (Throwable t) {
> + handleThrowable(t);
In this case, surely it would make more sense to just catch Exception?
I agree that it is vital that TD and VME are not ignored, but other
Throwables should be not be ignored either here.
> }
> }
> }
> @@ -375,6 +375,25 @@ public abstract class FileUploadBase {
> }
>
> /**
> + * Checks whether the supplied Throwable is one that needs to be
> + * rethrown and swallows all others.
> + * @param t the Throwable to check
> + */
> + private void handleThrowable(Throwable t) {
> + if (t instanceof ThreadDeath) {
> + throw (ThreadDeath) t;
> + }
> + if (t instanceof StackOverflowError) {
> + // Swallow silently - it should be recoverable
> + return;
> + }
> + if (t instanceof VirtualMachineError) {
> + throw (VirtualMachineError) t;
> + }
> + // All other instances of Throwable will be silently swallowed
> + }
> +
> + /**
> * Processes an <a href="http://www.ietf.org/rfc/rfc1867.txt">RFC 1867</a>
> * compliant <code>multipart/form-data</code> stream.
> *
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1638340 - in /commons/proper/fileupload/trunk/src:
changes/changes.xml main/java/org/apache/commons/fileupload/FileUploadBase.java
Posted by sebb <se...@gmail.com>.
On 2 December 2014 at 20:44, Thomas Neidhart <th...@gmail.com> wrote:
> On 12/02/2014 06:35 PM, sebb wrote:
>> On 11 November 2014 at 20:09, <tn...@apache.org> wrote:
>>> Author: tn
>>> Date: Tue Nov 11 20:09:32 2014
>>> New Revision: 1638340
>>>
>>> URL: http://svn.apache.org/r1638340
>>> Log:
>>> [FILEUPLOAD-242] Do not silently swallow all Throwables.
>>>
>>> Modified:
>>> commons/proper/fileupload/trunk/src/changes/changes.xml
>>> commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java
>>>
>>> Modified: commons/proper/fileupload/trunk/src/changes/changes.xml
>>> URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/changes/changes.xml?rev=1638340&r1=1638339&r2=1638340&view=diff
>>> ==============================================================================
>>> --- commons/proper/fileupload/trunk/src/changes/changes.xml (original)
>>> +++ commons/proper/fileupload/trunk/src/changes/changes.xml Tue Nov 11 20:09:32 2014
>>> @@ -44,6 +44,7 @@ The <action> type attribute can be add,u
>>>
>>> <body>
>>> <release version="1.4" date="TBA" description="TBA">
>>> + <action issue="FILEUPLOAD-242" dev="tn" type="fix">FileUploadBase - should not silently catch and ignore all Throwables</action>
>>> <action issue="FILEUPLOAD-257" dev="tn" type="fix">Fix Javadoc 1.8.0 errors</action>
>>> <action issue="FILEUPLOAD-234" dev="tn" type="fix">Fix section "Resource cleanup" of the user guide</action>
>>> <action issue="FILEUPLOAD-237" dev="tn" type="fix">Fix streaming example: use FileItem.getInputStream() instead of openStream()</action>
>>>
>>> Modified: commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java
>>> URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java?rev=1638340&r1=1638339&r2=1638340&view=diff
>>> ==============================================================================
>>> --- commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java (original)
>>> +++ commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java Tue Nov 11 20:09:32 2014
>>> @@ -366,8 +366,8 @@ public abstract class FileUploadBase {
>>> for (FileItem fileItem : items) {
>>> try {
>>> fileItem.delete();
>>> - } catch (Throwable e) {
>>> - // ignore it
>>> + } catch (Throwable t) {
>>> + handleThrowable(t);
>>
>> In this case, surely it would make more sense to just catch Exception?
>>
>> I agree that it is vital that TD and VME are not ignored, but other
>> Throwables should be not be ignored either here.
>
> I thought about that too but it was very unclear why it was catching
> Throwable in the first place.
It may just have been laziness.
> If you too think catching Exception would
> be sufficient than let's change it to that.
Yes, I think that would be enough.
Though I wonder why it does not report problems deleting the files.
AFAICT the FileCleaningTracker keeps track of files that it cannot delete.
> Thomas
>
>>
>>> }
>>> }
>>> }
>>> @@ -375,6 +375,25 @@ public abstract class FileUploadBase {
>>> }
>>>
>>> /**
>>> + * Checks whether the supplied Throwable is one that needs to be
>>> + * rethrown and swallows all others.
>>> + * @param t the Throwable to check
>>> + */
>>> + private void handleThrowable(Throwable t) {
>>> + if (t instanceof ThreadDeath) {
>>> + throw (ThreadDeath) t;
>>> + }
>>> + if (t instanceof StackOverflowError) {
>>> + // Swallow silently - it should be recoverable
>>> + return;
>>> + }
>>> + if (t instanceof VirtualMachineError) {
>>> + throw (VirtualMachineError) t;
>>> + }
>>> + // All other instances of Throwable will be silently swallowed
>>> + }
>>> +
>>> + /**
>>> * Processes an <a href="http://www.ietf.org/rfc/rfc1867.txt">RFC 1867</a>
>>> * compliant <code>multipart/form-data</code> stream.
>>> *
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> 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: svn commit: r1638340 - in /commons/proper/fileupload/trunk/src:
changes/changes.xml main/java/org/apache/commons/fileupload/FileUploadBase.java
Posted by Thomas Neidhart <th...@gmail.com>.
On 12/02/2014 06:35 PM, sebb wrote:
> On 11 November 2014 at 20:09, <tn...@apache.org> wrote:
>> Author: tn
>> Date: Tue Nov 11 20:09:32 2014
>> New Revision: 1638340
>>
>> URL: http://svn.apache.org/r1638340
>> Log:
>> [FILEUPLOAD-242] Do not silently swallow all Throwables.
>>
>> Modified:
>> commons/proper/fileupload/trunk/src/changes/changes.xml
>> commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java
>>
>> Modified: commons/proper/fileupload/trunk/src/changes/changes.xml
>> URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/changes/changes.xml?rev=1638340&r1=1638339&r2=1638340&view=diff
>> ==============================================================================
>> --- commons/proper/fileupload/trunk/src/changes/changes.xml (original)
>> +++ commons/proper/fileupload/trunk/src/changes/changes.xml Tue Nov 11 20:09:32 2014
>> @@ -44,6 +44,7 @@ The <action> type attribute can be add,u
>>
>> <body>
>> <release version="1.4" date="TBA" description="TBA">
>> + <action issue="FILEUPLOAD-242" dev="tn" type="fix">FileUploadBase - should not silently catch and ignore all Throwables</action>
>> <action issue="FILEUPLOAD-257" dev="tn" type="fix">Fix Javadoc 1.8.0 errors</action>
>> <action issue="FILEUPLOAD-234" dev="tn" type="fix">Fix section "Resource cleanup" of the user guide</action>
>> <action issue="FILEUPLOAD-237" dev="tn" type="fix">Fix streaming example: use FileItem.getInputStream() instead of openStream()</action>
>>
>> Modified: commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java
>> URL: http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java?rev=1638340&r1=1638339&r2=1638340&view=diff
>> ==============================================================================
>> --- commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java (original)
>> +++ commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/FileUploadBase.java Tue Nov 11 20:09:32 2014
>> @@ -366,8 +366,8 @@ public abstract class FileUploadBase {
>> for (FileItem fileItem : items) {
>> try {
>> fileItem.delete();
>> - } catch (Throwable e) {
>> - // ignore it
>> + } catch (Throwable t) {
>> + handleThrowable(t);
>
> In this case, surely it would make more sense to just catch Exception?
>
> I agree that it is vital that TD and VME are not ignored, but other
> Throwables should be not be ignored either here.
I thought about that too but it was very unclear why it was catching
Throwable in the first place. If you too think catching Exception would
be sufficient than let's change it to that.
Thomas
>
>> }
>> }
>> }
>> @@ -375,6 +375,25 @@ public abstract class FileUploadBase {
>> }
>>
>> /**
>> + * Checks whether the supplied Throwable is one that needs to be
>> + * rethrown and swallows all others.
>> + * @param t the Throwable to check
>> + */
>> + private void handleThrowable(Throwable t) {
>> + if (t instanceof ThreadDeath) {
>> + throw (ThreadDeath) t;
>> + }
>> + if (t instanceof StackOverflowError) {
>> + // Swallow silently - it should be recoverable
>> + return;
>> + }
>> + if (t instanceof VirtualMachineError) {
>> + throw (VirtualMachineError) t;
>> + }
>> + // All other instances of Throwable will be silently swallowed
>> + }
>> +
>> + /**
>> * Processes an <a href="http://www.ietf.org/rfc/rfc1867.txt">RFC 1867</a>
>> * compliant <code>multipart/form-data</code> stream.
>> *
>>
>>
>
> ---------------------------------------------------------------------
> 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