Community
Participate
Working Groups
Build Identifier: We package our RAP application as a war using the Equinox Servlet Bridge and we experience a "classloader leak" meaning that after undeploy of the application in tomcat the application classes are not removed from memory. This leads to PermGen OutOfMemory error after serveral restart or redeploy of the application. After investigating the problem it seems that the issue is that FileDialog implementation provided in the org.eclipse.rap.rwt.supplemental.filedialog relies on "Commons File Upload" which itself uses Commons IO FileCleaner API. The FileCleaner API starts a Thread that is never stopped and so prevents all application classes to be garbage collected. This thread should probably be stopped when stopping the FileDialog bundle. A possible workaround is to stop this thread when shutting down the application in a ServletContextListener using FileCleaner.getInstance().exitWhenFinished(). Reproducible: Always Steps to Reproduce: 1.Deploy and start a RAP application packaged as WAR in your servlet container 2.Use the FileDialog widget to upload a file 3.Stop the web application 4.Connect to your app server JVM using visualvm or similar and you'll see that a thread named "File Reaper" is still running
(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?
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?
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.
(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.
(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
(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()
(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.
(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.
Created attachment 206995 [details] Patch that cleans up for apache library
(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.
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.
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
The CQs have been approved, now we are waiting on the orbit bundle to be created/modified.
Orbit bundle has been updated.
Thanks for the patch. Changes are in CVS HEAD.
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).
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.
(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.
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).
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.
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.
(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.
Thanks a lot, that solved it.
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); } } ... }
A new simplified file cleaning implementation has been created - temporary files are deleted after their processing (at the end of every upload request).
