Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Tue, 5 Mar 2024 01:48:49 GMT, Jaikiran Pai wrote: >> Hi Jai. I think that solves all the functional requirements, but now we've >> turned two easily understood lines of code (calling run() and >> deleteIfExists()) into what you have above. I'm not so sure it is worth it. >> I refer back to Leonid's comment above on this topic: >> >>> I thought about execution of Files.deleteIfExists(path) in the case of test >>> fails, but decided that it is not so important. Also, test always might >>> fail with crash when no finally block is executed. So I am fine with >>> current way. > > Hello Chris, >> Hi Jai. I think that solves all the functional requirements, but now we've >> turned two easily understood lines of code (calling run() and >> deleteIfExists()) into what you have above. > > I agree. > >> I'm not so sure it is worth it. I refer back to Leonid's comment above on >> this topic: >> >> > I thought about execution of Files.deleteIfExists(path) in the case of >> > test fails, but decided that it is not so important. Also, test always >> > might fail with crash when no finally block is executed. So I am fine with >> > current way. > > What Leonid suggests looks fine to me and keeps things simple. Ok. I think then I'll just push this PR in its current form. - PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1513556350
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Mon, 4 Mar 2024 19:35:48 GMT, Chris Plummer wrote:
>> Hello Chris, I haven't followed this PR and this is just a drive by comment
>> about the exception handling discussion. In some other areas of our JDK
>> tests, for situations like this, we do the following:
>>
>>
>> Throwable failure = null;
>> try {
>> run(new JMXExecutor(), "Compiler.perfmap " + path.toString(), path);
>> } catch (Throwable t) {
>> failure = t;
>> } finally {
>> try {
>> Files.deleteIfExists(path);
>> } catch (IOException ioe) {
>> if (failure != null) {
>> // add the IOException as a suppressed exception to the original
>> failure
>> // and rethrow the original
>> failure.addSuppressed(ioe);
>> throw failure;
>> }
>> // no failures in the run(), but deleting the file failed, rethrow
>> this IOException
>> throw ioe;
>> }
>> }
>>
>> This way both the original test failure (if any) as well as the failure in
>> the finally block (if any) are propagated and made available in the failure
>> report.
>
> Hi Jai. I think that solves all the functional requirements, but now we've
> turned two easily understood lines of code (calling run() and
> deleteIfExists()) into what you have above. I'm not so sure it is worth it. I
> refer back to Leonid's comment above on this topic:
>
>> I thought about execution of Files.deleteIfExists(path) in the case of test
>> fails, but decided that it is not so important. Also, test always might fail
>> with crash when no finally block is executed. So I am fine with current way.
Hello Chris,
> Hi Jai. I think that solves all the functional requirements, but now we've
> turned two easily understood lines of code (calling run() and
> deleteIfExists()) into what you have above.
I agree.
> I'm not so sure it is worth it. I refer back to Leonid's comment above on
> this topic:
>
> > I thought about execution of Files.deleteIfExists(path) in the case of test
> > fails, but decided that it is not so important. Also, test always might
> > fail with crash when no finally block is executed. So I am fine with
> > current way.
What Leonid suggests looks fine to me and keeps things simple.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1512022330
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Sat, 2 Mar 2024 06:21:09 GMT, Jaikiran Pai wrote:
>> The one issue with the first variant is that when run() throws an exception,
>> it will be printed twice, assuming deleteIfExists() does not throw an
>> exception. Basically you either get the exception printed twice, or you get
>> the exception printed once plus whatever exception deleteIfExists() throws
>> also printed (which is ok).
>>
>> I think your 2nd example will work, but it's kind of confusing because if
>> you got into the finally block via an exception, you then might throw
>> another exception, which is handled in the finally block, and then
>> (implicitly) the original exception is rethrown as you exit the finally
>> block. However, if there was no exception from run() but deleteIfExists()
>> throws an exception, the exception is printed, but does not cause the test
>> to fail. I don't think we want that.
>
> Hello Chris, I haven't followed this PR and this is just a drive by comment
> about the exception handling discussion. In some other areas of our JDK
> tests, for situations like this, we do the following:
>
>
> Throwable failure = null;
> try {
> run(new JMXExecutor(), "Compiler.perfmap " + path.toString(), path);
> } catch (Throwable t) {
> failure = t;
> } finally {
> try {
> Files.deleteIfExists(path);
> } catch (IOException ioe) {
> if (failure != null) {
> // add the IOException as a suppressed exception to the original
> failure
> // and rethrow the original
> failure.addSuppressed(ioe);
> throw failure;
> }
> // no failures in the run(), but deleting the file failed, rethrow
> this IOException
> throw ioe;
> }
> }
>
> This way both the original test failure (if any) as well as the failure in
> the finally block (if any) are propagated and made available in the failure
> report.
Hi Jai. I think that solves all the functional requirements, but now we've
turned two easily understood lines of code (calling run() and deleteIfExists())
into what you have above. I'm not so sure it is worth it. I refer back to
Leonid's comment above on this topic:
> I thought about execution of Files.deleteIfExists(path) in the case of test
> fails, but decided that it is not so important. Also, test always might fail
> with crash when no finally block is executed. So I am fine with current way.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1511697784
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Sat, 2 Mar 2024 04:33:11 GMT, Chris Plummer wrote:
>> Two variants:
>>
>> public void runHelper(CommandExecutor executor, String cmd, Path path) {
>>try {
>>run(executor, cmd, path);
>>} catch (Throwable t) {
>> t.printStackTrace(System.out);
>> throw t;
>>} finally {
>>Files.deleteIfExists(path);
>>}
>> }
>>
>> or
>>
>> public void runHelper(CommandExecutor executor, String cmd, Path path) {
>>try {
>>run(executor, cmd, path);
>>} finally {
>>try {
>> Files.deleteIfExists(path);
>>} catch (Throwable t) {
>>
>> t.printStackTrace(System.out);
>>}
>>}
>> }
>
> The one issue with the first variant is that when run() throws an exception,
> it will be printed twice, assuming deleteIfExists() does not throw an
> exception. Basically you either get the exception printed twice, or you get
> the exception printed once plus whatever exception deleteIfExists() throws
> also printed (which is ok).
>
> I think your 2nd example will work, but it's kind of confusing because if you
> got into the finally block via an exception, you then might throw another
> exception, which is handled in the finally block, and then (implicitly) the
> original exception is rethrown as you exit the finally block. However, if
> there was no exception from run() but deleteIfExists() throws an exception,
> the exception is printed, but does not cause the test to fail. I don't think
> we want that.
Hello Chris, I haven't followed this PR and this is just a drive by comment
about the exception handling discussion. In some other areas of our JDK tests,
for situations like this, we do the following:
Throwable failure = null;
try {
run(new JMXExecutor(), "Compiler.perfmap " + path.toString(), path);
} catch (Throwable t) {
failure = t;
} finally {
try {
Files.deleteIfExists(path);
} catch (IOException ioe) {
if (failure != null) {
// add the IOException as a suppressed exception to the original
failure
// and rethrow the original
failure.addSuppressed(ioe);
throw failure;
}
// no failures in the run(), but deleting the file failed, rethrow this
IOException
throw ioe;
}
}
This way both the original test failure (if any) as well as the failure in the
finally block (if any) are propagated and made available in the failure report.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1509896340
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Sat, 2 Mar 2024 01:40:39 GMT, Serguei Spitsyn wrote:
>>> Q2: Can we use Files.deleteIfExists(path) in the finally block of the
>>> run() method instead?
>>
>> I don't think that changes anything. If run() throws an exception and then
>> Files.deleteIfExists() throws an exception from the finally block, the
>> original exception is lost.
>>
>>> One more approach is to wrap run() call into the runHelper() method:
>>
>> Same issue with potentially losing the original exception.
>
> Two variants:
>
> public void runHelper(CommandExecutor executor, String cmd, Path path) {
>try {
>run(executor, cmd, path);
>} catch (Throwable t) {
> t.printStackTrace(System.out);
> throw t;
>} finally {
>Files.deleteIfExists(path);
>}
> }
>
> or
>
> public void runHelper(CommandExecutor executor, String cmd, Path path) {
>try {
>run(executor, cmd, path);
>} finally {
>try {
> Files.deleteIfExists(path);
>} catch (Throwable t) {
>
> t.printStackTrace(System.out);
>}
>}
> }
The one issue with the first variant is that when run() throws an exception, it
will be printed twice, assuming deleteIfExists() does not throw an exception.
Basically you either get the exception printed twice, or you get the exception
printed once plus whatever exception deleteIfExists() throws also printed
(which is ok).
I think your 2nd example will work, but it's kind of confusing because if you
got into the finally block via an exception, you then might throw another
exception, which is handled in the finally block, and then (implicitly) the
original exception is rethrown as you exit the finally block. However, if there
was no exception from run() but deleteIfExists() throws an exception, the
exception is printed, but does not cause the test to fail. I don't think we
want that.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1509883828
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Sat, 2 Mar 2024 00:44:02 GMT, Chris Plummer wrote:
>> test/hotspot/jtreg/serviceability/dcmd/compiler/PerfMapTest.java line 103:
>>
>>> 101: } while(Files.exists(path));
>>> 102: run(new JMXExecutor(), "Compiler.perfmap " + path.toString(),
>>> path);
>>> 103: }
>>
>> Q1: It is not clear why the file is not deleted in this case. Do I miss
>> anything?
>> Q2: Can we use `Files.deleteIfExists(path)` in the `finally block` of the
>> `run()` method instead?
>>
>>
>> public void run(CommandExecutor executor, String cmd, Path path) {
>>try {
>>
>>} finally {
>>Files.deleteIfExists(path);
>>}
>> }
>>
>> One more approach is to wrap `run()` call into the `runHelper()` method:
>>
>> public void runHelper(CommandExecutor executor, String cmd, Path path) {
>>try {
>>run(executor, cmd, path);
>>} finally {
>>Files.deleteIfExists(path);
>>}
>> }
>
>> Q2: Can we use Files.deleteIfExists(path) in the finally block of the run()
>> method instead?
>
> I don't think that changes anything. If run() throws an exception and then
> Files.deleteIfExists() throws an exception from the finally block, the
> original exception is lost.
>
>> One more approach is to wrap run() call into the runHelper() method:
>
> Same issue with potentially losing the original exception.
Two variants:
public void runHelper(CommandExecutor executor, String cmd, Path path) {
try {
run(executor, cmd, path);
} catch (Throwable t) {
t.printStackTrace(System.out);
throw t;
} finally {
Files.deleteIfExists(path);
}
}
or
public void runHelper(CommandExecutor executor, String cmd, Path path) {
try {
run(executor, cmd, path);
} finally {
try {
Files.deleteIfExists(path);
} catch (Throwable t) {
t.printStackTrace(System.out);
}
}
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1509803616
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Sat, 2 Mar 2024 00:14:22 GMT, Serguei Spitsyn wrote: > Q2: Can we use Files.deleteIfExists(path) in the finally block of the run() > method instead? I don't think that changes anything. If run() throws an exception and then Files.deleteIfExists() throws an exception from the finally block, the original exception is lost. > One more approach is to wrap run() call into the runHelper() method: Same issue with potentially losing the original exception. - PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1509758594
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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. Looks good but posted a question. test/hotspot/jtreg/serviceability/dcmd/compiler/PerfMapTest.java line 103: > 101: } while(Files.exists(path)); > 102: run(new JMXExecutor(), "Compiler.perfmap " + path.toString(), > path); > 103: } Q: It is not clear why the file is not deleted in this case. Do I miss anything? - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17992#pullrequestreview-1912339995 PR Review Comment: https://git.openjdk.org/jdk/pull/17992#discussion_r1509721128
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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. I thought about execution of Files.deleteIfExists(path) in the case of test fails, but decided that it is not so important. Also, test always might fail with crash when no finally block is executed. So I am fine with current way. - PR Comment: https://git.openjdk.org/jdk/pull/17992#issuecomment-1974089555
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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. sorry, that's right the PidJcmdExecutor can't help get the error from the perfmap command, but it does capture some output, the jcmd confirmation. So it behaves differently, and will fail here as it checks the outputs are empty... - PR Comment: https://git.openjdk.org/jdk/pull/17992#issuecomment-1971608154
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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. So the STDOUT: and STDERR: messages are from jtreg for the test process itself, which is also including any stdout and stderr output produced while executing dcmd using jmx. So following message is on STDOUT: `[3.000s][warning][codecache] Failed to create /tmp/perf-.map for perf map` And the following message is to the target/test process STDERR: `Java HotSpot(TM) 64-Bit Server VM warning: Cannot open file /tmp/perf-.map due to Permission denied` The dcmd output is captured separately and is referred to as being on stdout and stderr. These are the ones we check to make sure they are empty, and unfortnately they are empty when there is an error because the messages were not sent to the dcmd output stream. I don't see how using PidJcmdExecutor helps. Doesn't it suffer from the same issue w.r.t. the warning/error messages missing? The output you are showing in your example comes from the jcmd process itself, not from the target VM. - PR Comment: https://git.openjdk.org/jdk/pull/17992#issuecomment-1971562377
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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. Created a separate issue: JDK-8327054 - PR Comment: https://git.openjdk.org/jdk/pull/17992#issuecomment-1971236277
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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. aha, sanity restored. This test uses JMXExecutor for the tests. If I make one of them use new PidJcmdExecutor() instead, the output is captured as we expect. But then it fails because there is some output there, like... stdout 28821: Command executed successfully stderr This looks like a separate issue 8-) - PR Comment: https://git.openjdk.org/jdk/pull/17992#issuecomment-1971081565
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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. Ok, so I'm not going insane and really did witness what I described, but I don't understand why. I create an empty file with root ownership: $ ls -lat /tmp/perf-* -rw-r--r-- 1 root root 0 Feb 28 19:35 /tmp/perf-.map And I get: test PerfMapTest.specifiedDefaultMapFile(): failure java.lang.AssertionError: File must not be empty. Possible file permission issue: /tmp/perf-.map expected [true] but found [false] So my empty file check is being triggered. Also in the log I see: test PerfMapTest.defaultMapFile(): success Running DCMD 'Compiler.perfmap /tmp/perf-.map' through 'JMXExecutor' [3.000s][warning][codecache] Failed to create /tmp/perf-.map for perf map stdout stderr But this output is not being detected by: OutputAnalyzer output = executor.execute(cmd); output.stderrShouldBeEmpty(); output.stdoutShouldBeEmpty(); It looks like stdout and stderr are empty. To add to the confusion, at the end of the test I see: STDERR: Java HotSpot(TM) 64-Bit Server VM warning: Cannot open file /tmp/perf-.map due to Permission denied This must be a testng issue. I don't deal with testng often, so I'm not sure how it might be handling output differently. But clearly there is output on stdout and/or stderr, and it is not detected. - PR Comment: https://git.openjdk.org/jdk/pull/17992#issuecomment-1969782466
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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. Marked as reviewed by lmesnik (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17992#pullrequestreview-1907197774
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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. Hmm. I'm starting to question everything I wrote in the 2nd paragraph of the description. I'm not sure how I came to that conclusion. If the file already exists and the current user does not have access to overwrite it, then as you point out, there will be an error message in stderr, and when stderr is not empty, the test fails. It would never hit the size check I added, and will never get an exception when calling `Files.deleteIfExists()` because the test would never get that far. Yet I'm sure at some point I got `Files.deleteIfExists()` to throw an exception. I need to figure out how I did that. - PR Comment: https://git.openjdk.org/jdk/pull/17992#issuecomment-1969692556
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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. Looks good. Adding the file size check is great. Just a nit on the message, if you are interested: I was wondering if "Possible file permission issue" isn't the right error for seeing a zero byte file? Fine to just say that it's a zero byte file. Seems like a permission failure would not create, or not truncate the file. If somebody else created the file, and we fail to overwrite it, it's going to have whatever size they left it with. 8-) But then I notice that if it fails due to permissions, that should be caught by the checks that output is empty, as it should show: Java HotSpot(TM) 64-Bit Server VM warning: Cannot open file file.txt due to Permission denied ..so it shouldn't get this far if jcmd printed an error. - Marked as reviewed by kevinw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17992#pullrequestreview-1905966982
Re: RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
On Fri, 23 Feb 2024 21:55:15 GMT, Chris Plummer 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-.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. Looks good. Please update copyright year before push - Marked as reviewed by amenkov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17992#pullrequestreview-1905142893
RFR: 8325532: serviceability/dcmd/compiler/PerfMapTest.java leaves created files in the /tmp dir.
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-.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. - Commit messages: - Delete tmp files Changes: https://git.openjdk.org/jdk/pull/17992/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17992&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325532 Stats: 11 lines in 1 file changed: 8 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17992.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17992/head:pull/17992 PR: https://git.openjdk.org/jdk/pull/17992
