You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Bernd Eckenfels <ec...@zusammenkunft.net> on 2014/02/15 22:29:04 UTC
Re: svn commit: r1568678 - in /commons/proper/fileupload/trunk/src:
changes/changes.xml
main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
Am Sat, 15 Feb 2014 21:18:43 -0000
schrieb sebb@apache.org:
> Author: sebb
> Date: Sat Feb 15 21:18:42 2014
> New Revision: 1568678
>
> URL: http://svn.apache.org/r1568678
> Log:
> FILEUPLOAD-245 DiskFileItem.get() may not fully read the data
>
> Modified:
> commons/proper/fileupload/trunk/src/changes/changes.xml
> commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.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=1568678&r1=1568677&r2=1568678&view=diff
> ==============================================================================
> --- commons/proper/fileupload/trunk/src/changes/changes.xml
> (original) +++
> commons/proper/fileupload/trunk/src/changes/changes.xml Sat Feb 15
> 21:18:42 2014 @@ -45,7 +45,8 @@ The <action> type attribute can be
> add,u <body> <release version="1.4" date="TBA" description= "TBA">
> - <action dev="sebb" type="update" due-to="Ville Skyttä">Make
> some MultipartStream private fields final</action>
> + <action issue="FILEUPLOAD-245" dev="sebb"
> type="fix">DiskFileItem.get() may not fully read the data</action>
> + <action issue="FILEUPLOAD-243" dev="sebb" type="update"
> due-to="Ville Skyttä">Make some MultipartStream private fields
> final</action> </release>
> <release version="1.3.1" description=
>
> Modified:
> commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
> URL:
> http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java?rev=1568678&r1=1568677&r2=1568678&view=diff
> ==============================================================================
> ---
> commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
> (original) +++
> commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
> Sat Feb 15 21:18:42 2014 @@ -312,7 +312,7 @@ public class
> DiskFileItem try { fis = new BufferedInputStream(new
> FileInputStream(dfos.getFile()));
> - fis.read(fileData);
> + IOUtils.readFully(fis, fileData);
There is no need to buffer the Streams if you are going
to read with large (maximum) buffers. I also wonder if this is really a
good idea to read the data fully into memory, only to stream it then to
somewhere else.
> } catch (IOException e) {
> fileData = null;
> } finally {
> @@ -681,9 +681,11 @@ public class DiskFileItem
> if (cachedContent != null) {
> output.write(cachedContent);
> } else {
> + new Throwable(dfosFile.toString()).printStackTrace();
> FileInputStream input = new FileInputStream(dfosFile);
> IOUtils.copy(input, output);
> - dfosFile.delete();
> + input.close();
> + System.out.println("deleted?"+dfosFile.delete()+"
> "+dfosFile); dfosFile = null;
Debug code?
Bernd
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1568678 - in /commons/proper/fileupload/trunk/src:
changes/changes.xml main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
Posted by sebb <se...@gmail.com>.
On 15 February 2014 23:14, Bernd Eckenfels <ec...@zusammenkunft.net> wrote:
> Hello,
>
> (pruning conversation)
>
> Am Sat, 15 Feb 2014 21:43:36 +0000 schrieb sebb <se...@gmail.com>:
>> On 15 February 2014 21:29, Bernd Eckenfels <ec...@zusammenkunft.net>
>> > I also wonder if this is really a
>> > good idea to read the data fully into memory, only to stream it
>> > then to somewhere else.
>>
>> The method in question returns the content of the file as an array of
>> bytes. It is a public method, so the API cannot readily be changed.
>
> True (luckily it gets internally only used in the write() method when the inMemory case is hit).
>
> But I suggest to add some warning to the 3 methods get/getString (the class javadoc mentions the problem but it is better to repeat it):
>
> Also the null return on IO errors is unfortunate, but since it cannot easily be changed, it should at least be documented:
>
> 295 /**
> 296 * Returns the contents of the file as an array of bytes. If the
> 297 * contents of the file were not yet cached in memory, they will be
> 298 * loaded from the disk storage and cached.
> 299 *
> 300 * @return The contents of the file as an array of bytes.
> 301 */
>
> new:
>
> 295 /**
> 296 * Returns the contents of the file as an array of bytes. If the
> 297 * contents of the file were not yet cached in memory, they will be
> 298 * loaded from the disk storage and cached. Be careful in requesting
> * the in-memory representation for larger files.
> 299 *
> 300 * @return The contents of the file as an array of bytes. This might
> * return <code>null</code> in case of I/O Exceptions while reading
> * the data from disk.
> 301 */
>
> Does that require a JIRA/Changelog entry or can I commit that without?
I don't think it's usually necessary to create JIRAs for Javadoc clarifications.
> The getString() and getString(String) methods do not handle the above mentioned null case. At least we should document that it can throw a NPE, but most likely thats not a good thing to keep (no matter how compatible or not).
The class does not handle null fields well; there are lots of cases
where NPEs can occur if methods are not called in the expected order
(FILEUPLOAD-247)
> ---------------------------------------------------------------------
> 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: r1568678 - in /commons/proper/fileupload/trunk/src:
changes/changes.xml
main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
Posted by Bernd Eckenfels <ec...@zusammenkunft.net>.
Hello,
(pruning conversation)
Am Sat, 15 Feb 2014 21:43:36 +0000 schrieb sebb <se...@gmail.com>:
> On 15 February 2014 21:29, Bernd Eckenfels <ec...@zusammenkunft.net>
> > I also wonder if this is really a
> > good idea to read the data fully into memory, only to stream it
> > then to somewhere else.
>
> The method in question returns the content of the file as an array of
> bytes. It is a public method, so the API cannot readily be changed.
True (luckily it gets internally only used in the write() method when the inMemory case is hit).
But I suggest to add some warning to the 3 methods get/getString (the class javadoc mentions the problem but it is better to repeat it):
Also the null return on IO errors is unfortunate, but since it cannot easily be changed, it should at least be documented:
295 /**
296 * Returns the contents of the file as an array of bytes. If the
297 * contents of the file were not yet cached in memory, they will be
298 * loaded from the disk storage and cached.
299 *
300 * @return The contents of the file as an array of bytes.
301 */
new:
295 /**
296 * Returns the contents of the file as an array of bytes. If the
297 * contents of the file were not yet cached in memory, they will be
298 * loaded from the disk storage and cached. Be careful in requesting
* the in-memory representation for larger files.
299 *
300 * @return The contents of the file as an array of bytes. This might
* return <code>null</code> in case of I/O Exceptions while reading
* the data from disk.
301 */
Does that require a JIRA/Changelog entry or can I commit that without?
The getString() and getString(String) methods do not handle the above mentioned null case. At least we should document that it can throw a NPE, but most likely thats not a good thing to keep (no matter how compatible or not).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1568678 - in /commons/proper/fileupload/trunk/src:
changes/changes.xml main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
Posted by sebb <se...@gmail.com>.
On 15 February 2014 21:29, Bernd Eckenfels <ec...@zusammenkunft.net> wrote:
> Am Sat, 15 Feb 2014 21:18:43 -0000
> schrieb sebb@apache.org:
>
>> Author: sebb
>> Date: Sat Feb 15 21:18:42 2014
>> New Revision: 1568678
>>
>> URL: http://svn.apache.org/r1568678
>> Log:
>> FILEUPLOAD-245 DiskFileItem.get() may not fully read the data
>>
>> Modified:
>> commons/proper/fileupload/trunk/src/changes/changes.xml
>> commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.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=1568678&r1=1568677&r2=1568678&view=diff
>> ==============================================================================
>> --- commons/proper/fileupload/trunk/src/changes/changes.xml
>> (original) +++
>> commons/proper/fileupload/trunk/src/changes/changes.xml Sat Feb 15
>> 21:18:42 2014 @@ -45,7 +45,8 @@ The <action> type attribute can be
>> add,u <body> <release version="1.4" date="TBA" description= "TBA">
>> - <action dev="sebb" type="update" due-to="Ville Skyttä">Make
>> some MultipartStream private fields final</action>
>> + <action issue="FILEUPLOAD-245" dev="sebb"
>> type="fix">DiskFileItem.get() may not fully read the data</action>
>> + <action issue="FILEUPLOAD-243" dev="sebb" type="update"
>> due-to="Ville Skyttä">Make some MultipartStream private fields
>> final</action> </release>
>> <release version="1.3.1" description=
>>
>> Modified:
>> commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
>> URL:
>> http://svn.apache.org/viewvc/commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java?rev=1568678&r1=1568677&r2=1568678&view=diff
>> ==============================================================================
>> ---
>> commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
>> (original) +++
>> commons/proper/fileupload/trunk/src/main/java/org/apache/commons/fileupload/disk/DiskFileItem.java
>> Sat Feb 15 21:18:42 2014 @@ -312,7 +312,7 @@ public class
>> DiskFileItem try { fis = new BufferedInputStream(new
>> FileInputStream(dfos.getFile()));
>> - fis.read(fileData);
>> + IOUtils.readFully(fis, fileData);
>
> There is no need to buffer the Streams if you are going
> to read with large (maximum) buffers.
That could be changed, but is a separate issue from ensuring that the
data is read fully.
> I also wonder if this is really a
> good idea to read the data fully into memory, only to stream it then to
> somewhere else.
The method in question returns the content of the file as an array of bytes.
It is a public method, so the API cannot readily be changed.
>
>> } catch (IOException e) {
>> fileData = null;
>> } finally {
>> @@ -681,9 +681,11 @@ public class DiskFileItem
>> if (cachedContent != null) {
>> output.write(cachedContent);
>> } else {
>> + new Throwable(dfosFile.toString()).printStackTrace();
>> FileInputStream input = new FileInputStream(dfosFile);
>> IOUtils.copy(input, output);
>> - dfosFile.delete();
>> + input.close();
>> + System.out.println("deleted?"+dfosFile.delete()+"
>> "+dfosFile); dfosFile = null;
>
> Debug code?
Oops, good catch.
> Bernd
>
> ---------------------------------------------------------------------
> 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