On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer <[email protected]> wrote:
> PerfMapTest.java issues the Compiler.perfmap jcmd with a filename argument to
> write the perfmap to. It does this in 3 different modes. 2 of the modes
> result in a perfmap file being left in the tmp directory that is not removed
> after the test executes (and should be removed). The 3rd mode creates the
> perfmap file in the directory specified by the test.dir property, and is ok
> to leave the file there. I've added code to delete the /tmp files that are
> created.
>
> I did a bit of extra testing by hand. I created `/tmp/perf-<pid>.map` as
> root. As expected the Files.deleteIfExists() call threw an exception due to
> the lack of permissions to delete the file. However, I then realized the file
> had a size of 0, which means the test was not even doing a proper job of
> testing that the perfrmap jcmd was working. In other words, the test should
> have failed long before getting to the Files.deleteIfExists(), so I added a
> size check to make sure it fails when the file is not written to.
The error handling in this test is a real PITA. I just realized that
Files.deleteIfExists() should really be in a finally block. But doing so means
that if it throws an exception, then any exception that was already thrown
during run() is lost. So, for example, let's say you don't have access to the
file and it already has contents, but they are not valid for a perfmap file.
The "Invalid file format" assert will result in an exception, but this would be
lost when Files.deleteIfExists() also throws a permissions exception. The way
around this is really ugly:
try {
run(new JMXExecutor(), "Compiler.perfmap " + path.toString(), path);
Files.deleteIfExists(path);
} catch (Throwable t) {
t.printStackTrace(System.out);
Files.deleteIfExists(path);
throw t;
}
So if run() passes, we do the deleteIfExists() and allow it to throw an
exception if needed (which would cause the test to appropriately fail with this
exception). If run() throws an exception, we catch it, print it, call
deleteIfExists(), and then rethrow the exception. If deleteIfExists() throws an
exception, then that is what the test will exit with, but at least we'll have
first printed the exception from run(), so it is not lost. However, if
deleteIfExists() doesn't throw an exception, we end up printing the exception
thrown by run() twice. Once in this added exception handler, and once by jtreg
on test exit. Can't say I like this approach, but I don't have a better
solution ATM.
The other choice is what we have now. Don't put deleteIfExists() in a finally
block, and don't bother deleting the file if run() throws an exception. The
main case where this becomes a problem is if the file is accessible, is written
to by the jcmd, but the contents for some reason don't pass the sanity test.
The test will appropriate fail, but it will also leave the perfmap file behind.
Maybe this is acceptable since it should never happen, and does help keep the
test code a lot simpler.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/17992#issuecomment-1973983886
