Commons FileUpload
  1. Commons FileUpload
  2. FILEUPLOAD-141

Remove FileItems if FileUploadBase.parseRequest() fails

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Duplicate
    • Affects Version/s: 1.2
    • Fix Version/s: 1.2.2
    • Labels:
      None
    • Environment:

      commons-fileupload is used for parsing multipart/form-data POST requests in servlets.
      OS: Linux

      Description

      If the method FileUploadBase.parseRequest() throws a FileUploadException, the already parsed FileItem objects are not accessible and removed by the garbage collector. Now expect a fileupload that fills the servers hard disc with FileItems until no space is left on the device. The method parseRequest() throws a FileUploadException and there are several FileItem objects that still exist in the device because the garbage collector does not run and removes them. This causes failing fileuploads until the garbage collector runs and removes the lost FileItem objects. I suggest calling FileItem.delete() on all FileItem objects created in the method FileUploadBase.parseRequest() if the method is left with a FileUploadException.

        Issue Links

          Activity

          Thomas Neidhart made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Fix Version/s 1.2.2 [ 12315238 ]
          Resolution Duplicate [ 3 ]
          Thomas Neidhart made changes -
          Link This issue duplicates FILEUPLOAD-160 [ FILEUPLOAD-160 ]
          Hide
          Thomas Neidhart added a comment -

          This has already been fixed in FILEUPLOAD-160.

          Now temporary files are cleaned-up in a try/finally block of the parseRequest method if the upload was not successful.

          Show
          Thomas Neidhart added a comment - This has already been fixed in FILEUPLOAD-160 . Now temporary files are cleaned-up in a try/finally block of the parseRequest method if the upload was not successful.
          Hide
          Andrew M added a comment -

          I also need this feature. I think it's important since common-fileupload users need to deal with these temporary files. I don't want to have to write some dodgy bash script to do this clean up on the server when it can be handled by the web application.

          Can we make this issue a priority and get it pushed out in the next release? I'll be happy to help with testing.

          I see some work has been done on this on the latest code base in the trunk. Why is it that temporary files are being deleted when the application is destroyed. Surely it would be better to clean up files at the end of every session?

          Show
          Andrew M added a comment - I also need this feature. I think it's important since common-fileupload users need to deal with these temporary files. I don't want to have to write some dodgy bash script to do this clean up on the server when it can be handled by the web application. Can we make this issue a priority and get it pushed out in the next release? I'll be happy to help with testing. I see some work has been done on this on the latest code base in the trunk. Why is it that temporary files are being deleted when the application is destroyed. Surely it would be better to clean up files at the end of every session?
          Hide
          Paul Benedict added a comment -

          Here's a past ticket from Struts on the very issue:
          https://issues.apache.org/struts/browse/STR-3031

          Show
          Paul Benedict added a comment - Here's a past ticket from Struts on the very issue: https://issues.apache.org/struts/browse/STR-3031
          Henri Yandell made changes -
          Resolution Won't Fix [ 2 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Hide
          Henri Yandell added a comment -

          (Reopening as closed issues with ongoing conversations are too easy to lose track of)

          Show
          Henri Yandell added a comment - (Reopening as closed issues with ongoing conversations are too easy to lose track of)
          Hide
          Paul Benedict added a comment - - edited

          That's why it should be an option. Adding a boolean property to control this behavior would be a minor enhancement. I've seen this issue come up before in server environments, so this request is definitely not unusual. And in fact, this would be a feature I would want to use often with my own customers. To have it supported officially by Commons Upload would be beneficial and, I think, warranted.

          Show
          Paul Benedict added a comment - - edited That's why it should be an option. Adding a boolean property to control this behavior would be a minor enhancement. I've seen this issue come up before in server environments, so this request is definitely not unusual. And in fact, this would be a feature I would want to use often with my own customers. To have it supported officially by Commons Upload would be beneficial and, I think, warranted.
          Hide
          Jochen Wiedmann added a comment -

          The fact that you want the files to be deleted is, IMO, by no means an indicator that others do wish the same.

          Show
          Jochen Wiedmann added a comment - The fact that you want the files to be deleted is, IMO, by no means an indicator that others do wish the same.
          Hide
          Marcus Klein added a comment -

          I think that the commons-fileupload API is especially used in server environments. Hanging files are always an issue in server environments in my opinion. Every unused file descriptor wastes resources in a server environment that may lead to problems due to always limited system resources.

          Can you please explain the compatibility problems that will occur with my suggested changes? How could cause calling delete() on not referenced FileItem objects any problems?

          Show
          Marcus Klein added a comment - I think that the commons-fileupload API is especially used in server environments. Hanging files are always an issue in server environments in my opinion. Every unused file descriptor wastes resources in a server environment that may lead to problems due to always limited system resources. Can you please explain the compatibility problems that will occur with my suggested changes? How could cause calling delete() on not referenced FileItem objects any problems?
          Jochen Wiedmann made changes -
          Field Original Value New Value
          Resolution Won't Fix [ 2 ]
          Status Open [ 1 ] Resolved [ 5 ]
          Hide
          Jochen Wiedmann added a comment -

          You are free to overwrite the FileItemFactory to return an instance of DiskFileItem, which overrides the method getTempFile() in that sense.

          Apart from that, changing the current code in your sense would most possibly be the cause of a lot of compatibility problems without gaining too much. I am unaware of any actual FileUpload installation that considers hanging files a real issue. This might be the case in your particular application, but then I believe it's fine that you tune the code to meet your special requirements.

          Show
          Jochen Wiedmann added a comment - You are free to overwrite the FileItemFactory to return an instance of DiskFileItem, which overrides the method getTempFile() in that sense. Apart from that, changing the current code in your sense would most possibly be the cause of a lot of compatibility problems without gaining too much. I am unaware of any actual FileUpload installation that considers hanging files a real issue. This might be the case in your particular application, but then I believe it's fine that you tune the code to meet your special requirements.
          Marcus Klein created issue -

            People

            • Assignee:
              Unassigned
              Reporter:
              Marcus Klein
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development