[fileupload] FileItem implements Serializable incorrectly

• Key: FILEUPLOAD-50
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Cris Daniluk

Environment

Operating System: All
Platform: PC

Description

org.apache.commons.fileupload.FileItem implements Serializable, but does not
honor the contract. It contains member field
org.apache.commons.fileupload.DeferredFileOutputStream, which is not
Serializable. Would prefer to have this class be truly Serializable but may not
be practical considering the nature of it. It should at least not define itself
as such, as it is misleading.

Comments

[Christoffer Hammarström]

Is there any hope that this will be fixed in the next release? This is the only
thing that makes my webapp sessions non-serializable.

[Martin Cooper]

I believe you are referring to DiskFileItem not being serialisable. I don't see
a way to fix this, as you say, because of the nature of the class. Note that it
is the FileItem interface that is serialisable, but this particular
implementation of that interface cannot be serialised. That doesn't mean that
all implementations will not be serialisable.

As for comment #1, the simple solution is to not store FileItem instances in
your session.

If anyone has any good ideas on how to make DiskFileItem serialisable, please
let me know ASAP. Otherwise I will mark this as WONTFIX.

[Niall Pemberton]

Created an attachment (id=17057)
Patch to make DiskFileItem Serializable

I'm attaching a patch that makes DiskFileItem serializable. It works by making
the DeferredFileOutputStream transient and storing the file contents in a cache
during serialization and then re-writing them back out (if not in memory) to a
new file during de-serialization. Its not ideal, but maybe better than not
being serializabe.

I also included a slight change to ensure cachedData is always used if exists.

[Niall Pemberton]

Created an attachment (id=17058)
Serialization JUnit test for DiskFileItem

[Martin Cooper]

Unfortunately, this will almost certainly cause OutOfMemory exceptions when
large uploads are involved. This is because both writeObject and readObject rely
on the entire contents being loaded into memory. That's fine for typical form
fields, but if the item represents a 2GB upload, things don't look so good.

Something along these lines that streams the content from or to a DFOS might
work, but I hate to think of the performance implications of serialisable
sessions that contain multiple gigabytes of data...

[Niall Pemberton]

Created an attachment (id=17059)
Modified patch for DiskFileItem

OK, how about copying the disk file to a new file during de-serialization
Instead?

[Martin Cooper]

That'll work. Thanks! Patch applied, with a simplification to readObject() and
some other minor changes.

[Niall Pemberton]

Created an attachment (id=17060)
3rd Patch for DiskFileItem

I guess the 2nd patch isn't a great idea either since its not v.efficient
copying a ton of data.

Attaching a 3rd patch - if its not "in memory" this one caches the File that
DeferredFileOutputStream has written to when serializing. Everywhere that
referenced the DeferredFileOutputStream now checks to see if that File is there
first. If it is "in memory" it re-creates the DeferredFileOutputStream.