[JENKINS-58393] Catch exception thrown by getTime()

[Dohbedoh]

JENKINS-58393: An exception may be thrown when doing a getTime() but is not caught in the loop on contents.

[Dohbedoh]

If we just add the content.getTime() inside the existing try / catch, an exception will be thrown here because there is not entry to close:

java.io.IOException: No current entry to close
	at org.apache.commons.compress.archivers.zip.ZipArchiveOutputStream.preClose(ZipArchiveOutputStream.java:532)
	at org.apache.commons.compress.archivers.zip.ZipArchiveOutputStream.closeArchiveEntry(ZipArchiveOutputStream.java:486)
	at com.cloudbees.jenkins.support.SupportPlugin.writeBundle(SupportPlugin.java:338)
	at com.cloudbees.jenkins.support.SupportPluginTest.testGenerateBundleExceptionHandler(SupportPluginTest.java:68)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:599)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.lang.Thread.run(Thread.java:748)

What I am proposing here is to log a warning that we could not set the time properly and try to print that content anyway.

[varyvol]

The fix looks good, but is this a realistic scenario? I mean when do you expect an exception being thrown when retrieving the time?

[Dohbedoh]

FileContent#getTime is never thrown. Which is why I removed it. FilePathContent#getTime() is what can throw an exception (reading the time through a remote channel).

[MRamonLeon]

Better to move entry.setTime(content.getTime()) into the try { on the next line

[MRamonLeon]

It happens only with contents based on Files and it's due to a lack of permissions if I understood correctly. For me, it's better to just move the instruction into the existing try { that avoids breaking the loop if something wrong happens with a specific content.

[Dohbedoh]

@MRamonLeon if an exception is raised before doing binaryOut.putArchiveEntry(entry), the method binaryOut.closeArchiveEntry() will fail:

                    try {
                        // if an exception is raised here
                        binaryOut.putArchiveEntry(entry); 
                        [...]
                    } catch (Throwable e) {                        
                        [...]
                    } finally {
                        [...]
                        binaryOut.closeArchiveEntry(); // This breaks with `java.io.IOException: No current entry to close`
                    }

[MRamonLeon]

If the exception is raised inside the method (which is possible) it's going to fail as well. I suppose bynaryOut.closeArchiveEntry(); should go at the end of the try section, but then the two previous sentences should go there too. The logic about these streams is very messy and deserves a bit of clean up. My concern is that if we cannot get the time of the entry, we are not going to be able to get the full content of that. Therefore, my proposal to include it all together in the same try. I've seen some implementation enclosing the closeArchiveEntry in a try-catch clause, but it's almost the same. So I leave it up to you.

[Dohbedoh]

My concern is that if we cannot get the time of the entry, we are not going to be able to get the full content of that.

You are right. I think in most cases, if we cannot get the time we cannot get the content.

[MRamonLeon]

Even the two previous instructions could be moved too

[Dohbedoh]

Looking into this, the putArchiveEntry is actually doing a closeArchiveEntry if necessary. So we actually don't need to do the binaryOut.closeArchiveEntry() in that finally as it would be taken care of by the next iteration if required. We should keep it in the try { ... } block nonetheless.

[Dohbedoh]

@MRamonLeon I am still a bit dubious about the streams reset. Maybe these 2 reset should always be done before the putArchiveEntry ?

[MRamonLeon]

@Dohbedoh we have to keep the closeArchiveEntry to close the latest entry of the loop. It may be in the finally or in the try.

Maybe these 2 reset should always be done before the putArchiveEntry ?
I would say they should go after the closeArchiveEntry actually.

All the logic related to the Streams is a bit messy FMPOV. I would suggest doing some tests moving the closeArichiveEntry up to the final of the try and getting the setTime into the try as well (maybe the name and entry too). If it doesn't work, keep the closeArchiveEntry in the finally but enclosing it with a try-catch or add a flag to know if the entry was created in the try and use that to do the closeArchiveEntry in the finally:

...
try {
   entry.setTime(content.getTime());
   entryCreated = true;
   ...

} finally {
   ...
   if (entryCreated) {
      binaryOut.closeArchiveEntry();
      entryCreated = false; //for next iteration
   }
}

My understanding is that the first option should work.

[Dohbedoh]

I am good with this. I added a flag and conditionals.

I would suggest doing some tests moving the closeArichiveEntry up to the final of the try and getting the setTime into the try as well (maybe the name and entry too)

I have done some tests, seemed to work. But I also not have been able to reproduce that IOE consistently. I feel like adding a flag is safer for now.

[MRamonLeon]

Great!