Bug 362924

Comment 1 Austin Riddle 2011-11-08 15:39:16 EST

(In reply to comment #0)
> This thread should probably be stopped when stopping the FileDialog bundle.

Hi Sebastien,

Thanks for opening this bug! 

One of the complications with calling exitWhenFinished() is where to put it.  Certain bundles (e.g., RWT, filedialog, fileupload) were designed to run without a dependency on the osgi runtime. So there is no bundle activator to manage a lifecycle.  Do you happen to know if the FileCleaner is recreated automatically if needed? That would allow us to call exitWhenFinished() at multiple times, for instance when a file dialog/file handler is disposed.  Does anyone else have an idea?

Comment 2 Sebastien Arod 2011-11-09 04:14:18 EST

Hi Austin,

I gave a quick look at commons IO source code and it seems that the file cleaner thread won't be restarted after calling exitWhenFinished().

However I still think this can be fixed at application/bundle shutdown by adding an Activator to manage the problem in OSGI environment and using the org.eclipse.rwt.engine.RWTServletContextListener for standalone web applications.

Am I missing something?

Comment 3 Sebastien Arod 2011-11-09 05:29:37 EST

I can still see a problem with the approach I proposed in OSGI environment because FileCleaner uses a singleton FileCleanerTracker:
If FileCleaner.exitWhenFinished() is called in an Activator.stop() for org.eclipse.rap.rwt.supplemental.fileupload bundle and if an other bundle is relying on FileCleaner the FileCleanerTracker will be stopped and files won't be cleaned up anymore for the other bundle.
Similarly if org.eclipse.rap.rwt.supplemental.fileupload bundle is stopped and then restarted the FileCleaner won't work anymore.

It seems that there is a new API available in the new version of Commons FileUpload (1.2.2) that allows to specify the FileCleanerTracker for a  DiskFileItemFactory. This would allow to manage a singleton at org.eclipse.rap.rwt.supplemental.fileupload bundle level instead of using the FileCleaner singleton.

Comment 4 Ralf Sternberg 2011-11-09 05:37:36 EST

(In reply to comment #2)
> However I still think this can be fixed at application/bundle shutdown by adding
> an Activator to manage the problem in OSGI environment and using the
> org.eclipse.rwt.engine.RWTServletContextListener for standalone web
> applications.

Hi Sebastien, as Austin said, RWT can (and actually is) also be used without OSGi. In this setup, the Activator would never be called.

Having a file cleaner thread around sounds a bit scary to me (maybe it isn't). Couldn't we disable this thing completely and cleanup ourselves, maybe simply using File#deleteOnExit?

FWIW, I've digged into the commons fileupload code when working on the server-side upload APIs, and I have to say that I'm not happy with it at all. It does funny things like accepting huge files even if a max file size is set, just to reject the file after it has been completely uploaded. I would like to get rid of this dependency and accept the upload ourselves. Sounds like re-inventing the wheel, but in fact, we only need a fraction of the functionality provided by apache fileupload which should be easy to implement.

Comment 5 Sebastien Arod 2011-11-09 06:06:03 EST

(In reply to comment #4)
> Hi Sebastien, as Austin said, RWT can (and actually is) also be used without
> OSGi. In this setup, the Activator would never be called.

Hi Ralph, 
I was thinking that RWTContextListener could be used to handle the shutdown in non-OSGI mode. I know that RWTContextListener and org.eclipse.rap.rwt.supplemental.fileupload are separate bundles but FileUploadProcessor could register a kind of shutdown hook in a RWTContextListener singleton for instance at first use.

There might be some problems to replace FileCleanerTracker with File.deleteOnExit():
1/ File.deleteOnExit() waits for actual JVM exit to cleanup which can be weeks for an application server
2/ It will consumes memory for every uploaded file until JVM is stopped. http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4513817

Comment 6 Ralf Sternberg 2011-11-09 06:23:25 EST

(In reply to comment #5)
> (In reply to comment #4)
> > Hi Sebastien, as Austin said, RWT can (and actually is) also be used without
> > OSGi. In this setup, the Activator would never be called.
> 
> Hi Ralph,
> I was thinking that RWTContextListener could be used to handle the shutdown in
> non-OSGI mode. I know that RWTContextListener and
> org.eclipse.rap.rwt.supplemental.fileupload are separate bundles but
> FileUploadProcessor could register a kind of shutdown hook in a
> RWTContextListener singleton for instance at first use.
> 
> There might be some problems to replace FileCleanerTracker with
> File.deleteOnExit():
> 1/ File.deleteOnExit() waits for actual JVM exit to cleanup which can be weeks
> for an application server
> 2/ It will consumes memory for every uploaded file until JVM is stopped.
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4513817

Good points. Then what about using a session store listener for cleanup?
  RWT.getSessionStore().addSessionStoreListener()

Comment 7 Sebastien Arod 2011-11-09 08:53:46 EST

(In reply to comment #6)
> Good points. Then what about using a session store listener for cleanup?
>   RWT.getSessionStore().addSessionStoreListener()

Using a sessionstorelistner to cleanup the files seems good to me.

Comment 8 Austin Riddle 2011-11-09 11:41:06 EST

(In reply to comment #6)
> Good points. Then what about using a session store listener for cleanup?
>   RWT.getSessionStore().addSessionStoreListener()

That sounds reasonable.

What about the most recent versions of the Apache libraries? Is anything 'cleaner' (ha ha) in those? I can submit a CQ for updating the approved orbit bundle versions.

Comment 9 Austin Riddle 2011-11-14 17:11:48 EST

Created attachment 206995 [details]
Patch that cleans up for apache library

Comment 10 Sebastien Arod 2011-11-15 03:03:41 EST

(In reply to comment #9)
Hi Austin,

I'm afraid that with the patch provided the cleanup will only work for the first user session because as soon as  FileCleaner.getInstance().exitWhenFinished(); is called the thread is terminated but future calls to FileCleaner.track(...) will not start a new cleaner.

If we want to be able to stop the FileCleanerTracker thread in RAP we should upgrade to FileUpload 1.2.2 that allows passing the FileCleanerTracker instance to the DiskFileItemFactory or re-implement file upload mechanism as Ralph suggested.

Comment 11 Sebastien Arod 2011-11-15 03:59:28 EST

Created attachment 207010 [details]
Patch using commons file upload 1.2.2 APIs

I attached a patch that use a FileCleaningTracker per UserSession where FileUpload is used and that desstroy it using a session listener.

Comment 12 Austin Riddle 2011-11-16 14:20:03 EST

I have opened a new CQ for approving the next version:
 https://dev.eclipse.org/ipzilla/show_bug.cgi?id=5809
 
I also have opened a bug for Orbit:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=363952

And Chris has opened his associated CQ:
https://dev.eclipse.org/ipzilla/show_bug.cgi?id=5810

Comment 13 Austin Riddle 2011-11-28 11:19:33 EST

The CQs have been approved, now we are waiting on the orbit bundle to be created/modified.

Comment 14 Austin Riddle 2011-12-23 06:32:13 EST

Orbit bundle has been updated.

Comment 15 Austin Riddle 2012-01-24 18:26:55 EST

Thanks for the patch. Changes are in CVS HEAD.

Comment 16 Ralf Sternberg 2012-02-01 12:09:57 EST

With this solution, not only the cleanup is done on the end of a user session, there's also one new thread being started per user session. This potentially doubles the number of threads running on the server. Since one single thread per application is completely sufficient I would opt for storing the reference in the application store. This leaves the problem of finding a suitable hook for its termination (see bug 370354).

Comment 17 Ralf Sternberg 2012-02-01 12:22:26 EST

Moreover, the new method FileUploadProcessor#getCleaningTracker() lacks proper synchronization. When a client starts multiple file uploads at once, a race condition could lead to creating two FileCleaningTrackers and attaching two SessionStoreListeners. On session end, the second listener would then cause a NPE because the attribute has already been removed.

Comment 18 Austin Riddle 2012-02-01 15:12:58 EST

(In reply to comment #16)
> Since one single thread per
> application is completely sufficient I would opt for storing the reference in
> the application store. This leaves the problem of finding a suitable hook for
> its termination (see bug 370354).
+1 to that.

Comment 19 Austin Riddle 2012-02-01 15:28:36 EST

Another option would be to construct an application-scoped lifecycle manager for FileCleanerTracker. The FileUploadProcessor could ask the manager for a cleaner. The manager would create a cleaner and keep a reference count and return the one instance each time. Then we could use the same session destroy hook, which would call the manager to handle disposal. When the refcount goes back to zero, the manager can do appropriate cleanup with the cleaner.  If another instance request comes in, the manger would create a new cleaner. In this case  we won't have a thread running at all when there are periods of inactivity (regarding fileupload).

Comment 20 Ralf Sternberg 2012-02-01 17:24:17 EST

Good idea! But before you start implementing a reference counter, let's wait if we can provide an application shutdown hook. This would be cleaner and less error prone than reference counting.
I extracted the handling of the FileCleaningTracker into a separate class for a start.

Comment 21 Stephan Leicht Vogt 2012-03-06 11:34:33 EST

There is a deadlock in the CleaningTrackerUtil: when entering stopCleaningTracker there is a synchronized(ISessionStore). This object is already locked and this synch is not necessary anyway because store.removeAttribute(String) already has a lock.

Solution: remove all synchronized in the CleaningTrackerUtil.

Comment 22 Ivan Furnadjiev 2012-03-06 11:40:43 EST

(In reply to comment #21)
> There is a deadlock in the CleaningTrackerUtil: when entering
> stopCleaningTracker there is a synchronized(ISessionStore). This object is
> already locked and this synch is not necessary anyway because
> store.removeAttribute(String) already has a lock.
> 
> Solution: remove all synchronized in the CleaningTrackerUtil.
The deadlock has been fixed in CVS HEAD - see bug 372946.

Comment 23 Stephan Leicht Vogt 2012-03-06 11:48:46 EST

Thanks a lot, that solved it.

Comment 24 Elias Volanakis 2012-11-06 19:49:25 EST

FYI - On RAP 1.5: The FileDialog appears to be leaking Reaper threads (they block in Line 22: run() { ...  Tracker tracker = (Tracker) q.remove(); ... and don't terminate). As a workaround I'm disabling the CleaningTracker and deleting the temp. dir manually, like so:

class MyFileUploadListener implements FileUploadListener {
  public void uploadFinished(FileUploadEvent event) {
    // Process fromFile ... then ...
    File fromFile = receiver.getTargetFile();
    File parent = fromFile.getParentFile();
    if (parent.getName().startsWith("fileupload_")) {
      FileUtils.deleteRec(parent);
    }
  }
  ...
}

Comment 25 Ivan Furnadjiev 2013-10-07 02:18:39 EDT

A new simplified file cleaning implementation has been created - temporary files are deleted after their processing (at the end of every upload request).