Comment 1 Thomas Watson 2008-11-20 09:47:59 EST

Thanks Gary, I released the patch.

I am wandering if you have observed any performance analysis that would indicate that we are leaking ZipInputStreams.  We use ZipInputStreams for every class and resource load.  If we are seeing real leaks in large numbers then I would expect it to come from that area.

Comment 2 Chris Grindstaff 2008-11-20 10:01:15 EST

I can speak for Notes - I see zero live instances of ZipInputStream after a 5 day longrun. No leaks there.

Comment 3 Gary Karasiuk 2008-11-20 10:08:35 EST

(In reply to comment #1)
> I am wandering if you have observed any performance analysis that would
> indicate that we are leaking ZipInputStreams.  We use ZipInputStreams for every
> class and resource load.  If we are seeing real leaks in large numbers then I
> would expect it to come from that area.
> 
In an adopting product, we are seeing thousands of instances of ZipInputStreams that are not being closed. The thing is, they don't show up in the Java Heap, the leak is only in native memory.

I'm trying to add some low level tracing to see where they are all coming from.

Comment 4 Thomas Watson 2008-11-20 10:32:05 EST

Gary, please set the following config.ini property and see if the leaks still occur:

osgi.bundlefile.limit=0

This will disable the dynamic closing of ZipFiles.  You could add some diagnostic code to:

org.eclipse.osgi.baseadaptor.bundlefile.ZipBundleFile.close()

We keep track of the number of open streams to the zip.  If the number of open steams is not 0 then we will not close the zip file.

I would set osgi.bundlefile.limit=0 first to see if there are still leaks.  If you still see the leaks then they are not related to this code.

Comment 5 Ed Merks 2008-11-29 09:45:31 EST

I wonder if code like this will have a problem.

  protected ImportOperation createZipImportOperation(ProjectDescriptor projectDescriptor, File file) throws Exception
  {
    ZipFile zipFile = file.getName().endsWith(".jar") ? new JarFile(file) : new ZipFile(file);
    ZipFileStructureProvider zipFileStructureProvider = new ZipFileStructureProvider(zipFile);

    return new ImportOperation(
      projectDescriptor.getProject().getFullPath(),
      zipFileStructureProvider.getRoot(),
      zipFileStructureProvider,
      OVERWRITE_ALL_QUERY);
  }

I.e., who's responsible for closing the ZipFile in this case?  I suppose it should be like this (though I've not tested it).

  protected ImportOperation createZipImportOperation(ProjectDescriptor projectDescriptor, File file) throws Exception
  {
    final ZipFile zipFile = file.getName().endsWith(".jar") ? new JarFile(file) : new ZipFile(file);
    ZipFileStructureProvider zipFileStructureProvider = new ZipFileStructureProvider(zipFile);

    return 
      new ImportOperation
       (projectDescriptor.getProject().getFullPath(),
        zipFileStructureProvider.getRoot(),
        zipFileStructureProvider,
        OVERWRITE_ALL_QUERY)
      {
        @Override
        protected void execute(IProgressMonitor progressMonitor)
        {
          try
          {
            super.execute(progressMonitor);
          }
          finally
          {
            try
            {
              zipFile.close();
            }
            catch (IOException exception)
            {
              // Ignore.
            }
          }
        }
      };
  }

I think this from org.eclipse.pde.internal.ui.samples.SampleOperation suffers the same problem.

	private void importFilesFromZip(ZipFile srcZipFile, IPath destPath, IProgressMonitor monitor) throws InvocationTargetException, InterruptedException {
		ZipFileStructureProvider structureProvider = new ZipFileStructureProvider(srcZipFile);
		ImportOperation op = new ImportOperation(destPath, structureProvider.getRoot(), structureProvider, query);
		op.run(monitor);
	}

I suspect that org.eclipse.ant.internal.ui.preferences.AddCustomDialog suffers from this problem too, i.e., it creates a ZipFile but doesn't close it as far as I can tell.

Comment 6 John Arthorne 2008-12-01 10:25:32 EST

Yes, I think the usual garbage collection rule of thumb applies here: if you opened it, you're reponsible for closing it.  In this case, the constructor of ZipFile and JarFile opens the file, so whoever constructs it should be finally closing it.

However the bigger leak Gary points out here is regarding closing the *streams* opened on the zip/jar.  Closing the jar/zip is required as well, although since the finalize method on ZipFile does this implicitly, it's likely not as big a source of leaks as the stream problem.