10 Replies Latest reply on Jan 15, 2009 9:57 AM by alesj

    Plague of temp VFS files and OOM on redeploy

    alesj

      WRT
      - http://www.jboss.com/index.html?module=bb&op=viewtopic&t=147622&start=20

      I already did some changes, but they are not solving all the problems users face.

      The problems:
      (1) expire of VFS cache entries lead to plague of temp files
      (2) the temp files are never deleted
      (3) re-deploy causes OOM on VFS code

      What I did and/or what is still to do:

      (1) I added Combined VFS cache where part of it is permanent entries (most common roots),
      the other part is real cache - any of the VFSCache impls

      (2) I'm currently looking into the issue,
      eventually closing VirtualFileHandler when it's no longer used
      e.g. in FileHandler update --> closing cached/old child handlers

      Current code keeps a ref track on VirtualFileHandler usage,
      only to allow close when ref == 0.
      But in FileSystemContext we keep root's VirtualFileHandler ref from the beginning.

       /** A reference to the virtual file of the root to stop it getting closed */
       private VirtualFile rootFile;
      
       public VirtualFileHandler getRoot() throws IOException
       {
       if (root == null)
       {
       root = createVirtualFileHandler(null, file);
       if (root == null)
       throw new java.io.FileNotFoundException((file == null ? "<null>" : file.getName())
       + " doesn't exist. (rootURI: " + getRootURI() + ", file: " + file + ")");
      
       rootFile = root.getVirtualFile();
      

      What's the purpose of this? (see comment/usage)

      This prevents me from closing something like this:
      - my.ear/ui.war/lib/tools.jar
      Where I would want to close my.ear's VirtualFile ref,
      eventually closing all nested entries -- actually also deleting its matching temp files.

      The new code to ZipEntryContext does that:
       public void close(ZipEntryHandler handler)
       {
       VirtualFileHandler rootHandler = getRoot();
       if (rootHandler.equals(handler))
       {
       getZipSource().close();
       // only close nested - as they might be temp files we want to delete
       for (VirtualFileHandler vfh : nestedHandlers)
       {
       vfh.close();
       }
       }
       }
      


      (3) I only got this via user feedback, hence not really tested
      And like the users says, the real cause might be elsewhere.
      But Marko promised to have a look anyway. :-)

      The user also notes that this wasn't an issue in previous JBossAS versions.
      But after starting small internal discussion I'm no longer persuaded that that's really true,
      as I didn't get any (internal) feedback that would point me to some real proof that we actually did things any different,
      different from File.deleteOnExit or deleting temp at boot time.
      It might be just the simplicity of what we did before that resulted in things working.

      Looks like that what might work for users, doesn't quite work for us yet
      - simplicity vs. VFS's API. But we'll get there. ;-)

        • 1. Re: Plague of temp VFS files and OOM on redeploy
          starksm64

           

          "alesj" wrote:

          (3) I only got this via user feedback, hence not really tested
          And like the users says, the real cause might be elsewhere.
          But Marko promised to have a look anyway. :-)

          The user also notes that this wasn't an issue in previous JBossAS versions.
          But after starting small internal discussion I'm no longer persuaded that that's really true,
          as I didn't get any (internal) feedback that would point me to some real proof that we actually did things any different,
          different from File.deleteOnExit or deleting temp at boot time.
          It might be just the simplicity of what we did before that resulted in things working.

          Looks like that what might work for users, doesn't quite work for us yet
          - simplicity vs. VFS's API. But we'll get there. ;-)

          The old MainDeployer has an explicit delete of existing content in the tmp directory when it was updated, so the question is how is the redeploy being done?

          • 2. Re: Plague of temp VFS files and OOM on redeploy
            alesj

             

            "scott.stark@jboss.org" wrote:

            The old MainDeployer has an explicit delete of existing content in the tmp directory when it was updated

            OK, I'll check how we used to do this in old MD.

            "scott.stark@jboss.org" wrote:

            so the question is how is the redeploy being done?

            What exactly do you mean here?

            • 3. Re: Plague of temp VFS files and OOM on redeploy
              alesj

               

              "alesj" wrote:
              "scott.stark@jboss.org" wrote:

              The old MainDeployer has an explicit delete of existing content in the tmp directory when it was updated

              OK, I'll check how we used to do this in old MD.

              We did this in DeploymentInfo::cleanup,
              which was called in MD's undeploy --> destroy.

              Should we have something similar in our DeployersImpl?
               unregisterMBean(context);
               removeClassLoader(context);
               cleanup(context); <-- HERE
              


              Currently at HERE we only do metadata cleanup.
              AbstractDeploymentContext:
               public void cleanup()
               {
               cleanupRepository(this);
               }
              

              Perhaps adding something like this in AbstractVFSDeploymentContext
               @Override
               public void cleanup()
               {
               super.cleanup();
               root.close(); // or adding new VirtualFile::cleanup() method?
               }
              



              • 4. Re: Plague of temp VFS files and OOM on redeploy
                alesj

                 

                "alesj" wrote:
                Perhaps adding something like this in AbstractVFSDeploymentContext
                 @Override
                 public void cleanup()
                 {
                 super.cleanup();
                 root.cleanup();
                 }
                


                I've tried this and it works. :-)

                I deployed Seam Booking|Dvd, which has a couple of nested jars --> temp copy.
                They were all nicely deleted at undeploy.

                • 5. Re: Plague of temp VFS files and OOM on redeploy
                  ropalka

                   

                  "alesj" wrote:

                   @Override
                   public void cleanup()
                   {
                   super.cleanup();
                   root.cleanup();
                   }
                  

                  I've tried this and it works. :-)

                  Always call super.cleanup() style methods finally!

                  • 6. Re: Plague of temp VFS files and OOM on redeploy
                    alesj

                     

                    "richard.opalka@jboss.com" wrote:

                    Always call super.cleanup() style methods finally!

                    You mean the order or try/finally block?
                    If the 2nd, it's not always true. ;-)

                    Imagine having a lot of things to call at cleanup,
                    you might end up with loads of try/finally's.
                    It's then better to 'enforce' a strict contract of suppressing exceptions
                    in the actual sub-cleanup methods.
                    And that's exactly what we do here.

                    But, since it's only a single sub-cleanup call,
                    I'll put it in try/finally (I actually already did this :-).
                    The order doesn't matter.

                    • 7. Re: Plague of temp VFS files and OOM on redeploy
                      ropalka

                      And how does your commit look like?
                      1.)

                      @Override
                      public void cleanup()
                      {
                       try
                       {
                       super.cleanup();
                       }
                       finally
                       {
                       root.cleanup();
                       }
                      }
                      

                      or 2.)
                      @Override
                      public void cleanup()
                      {
                       try
                       {
                       root.cleanup();
                       }
                       finally
                       {
                       super.cleanup();
                       }
                      }
                      


                      I'm asking because first approach is error prone (from maintainability point of view).
                      Seems we're starting to play catching game. I don't want to;)

                      • 8. Re: Plague of temp VFS files and OOM on redeploy
                        alesj

                         

                        "richard.opalka@jboss.com" wrote:

                        I'm asking because first approach is error prone (from maintainability point of view).
                        Seems we're starting to play catching game. I don't want to;)

                        Why is it error prone?
                        What catching game?


                        • 9. Re: Plague of temp VFS files and OOM on redeploy
                          ropalka

                          In this concrete case it's not the problem, because AbstractVFSDeploymentContext is a subclass of AbstractDeploymentContext and represents new and proper abstraction.
                          Generally speaking it's the problem in case users use improper abstractions. For example root in our case could be refactored to superclass in the future if AbstractVFSDeploymentContext wouldn't be proper abstraction.

                          • 10. Re: Plague of temp VFS files and OOM on redeploy
                            alesj

                             

                            "richard.opalka@jboss.com" wrote:

                            Generally speaking it's the problem in case users use improper abstractions.

                            Sure. ;-)
                            But just to use Adrian's words on you - I got them in the first place. :-)
                            - http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4201425#4201425
                            See the rope & hang. ;-)