PING: RFR: 8220657: JFR.dump does not work when filename is set

[Erik Gahlin]

Hi Yasumasa,

The test looks complicated.

I would prefer something more straight forward with less states and 
nulls where the expected behavior is easy to see. If the test fails, 
it's also nice to see from the stack trace which sub test failed.

   public static void main(String... args) throws Exception {
     testDumpAll();
     testDumpNamed();
     testDumpNamedWithFilename();
   }

   private static void testDumpAll() throws Exception {
     Path p = Path.of("A.jfr").toAbsolutePath();
     try (Recording r = new Recording()) {
       r.setName("A");
       r.setDestination(p);
       JcmdHelper.jcmd("JFR.dump");
       Asserts.assertFalse(namedFile(p), "Unexpected file: " + p);
       Asserts.assertTrue(generatedFile(), "Expected generated file");
     }
   }

   private static void testDumpNamed() throws Exception {
     Path p = Path.of("B.jfr").toAbsolutePath();
     try (Recording r = new Recording()) {
       r.setName("B");
       r.setDestination(p);
       JcmdHelper.jcmd("JFR.dump", "name=B");
       Asserts.assertTrue(namedFile(p), "Expected file: " + p);
       Asserts.assertFalse(generatedFile(), "Unexpected generated file");
     }
   }

   private static void testDumpNamedWithFilename() throws Exception {
     Path p = Path.of("C.jfr").toAbsolutePath();
     Path override = Path.of("override.jfr").toAbsolutePath();
     try (Recording r = new Recording()) {
       r.setName("C");
       r.setDestination(p);
       JcmdHelper.jcmd("JFR.dump", "name=C", "filename=" + override);
       Asserts.assertFalse(namedFile(p), "Unexpected file: " + p);
       Asserts.assertTrue(namedFile(p), "Expected file: " + override);
       Asserts.assertFalse(generatedFile(), "Unexpected generated file");
     }
   }

   private static boolean namedFile(Path dumpFile) throws IOException {
     return Files.exists(dumpFile) && Files.size(dumpFile) > 0;
   }

   private static boolean generatedFile() throws IOException {
     long pid = ProcessHandle.current().pid();
     return Files.find(Path.of("."), 1, (p, a) -> p.toString()
                 .matches("^.*hotspot-pid-" + pid + ".jfr$") && a.size() 
 > 0L)
                 .findFirst().isPresent();
   }

I haven't tried it.

When it comes to DCmdDump, I would prefer if you would not use "var" as 
this hides the underlying type (SafePath or WriteableUserPath) which is 
used to signal if there is a security risk or not. I think it is fine, 
but if the code gets refactor at a later stage the information could get 
lost.

Thanks
Erik

> PING: Could you review it?
>
> > http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.02/
>
>
> Yasumasa
>
>
> On 2019/04/24 21:43, Yasumasa Suenaga wrote:
>> Hi Erik,
>>
>> I uploaded new webrev:
>>
>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.02/
>>
>> I kept to remove setDestination() call in PlatformRecording.
>> In this webrev, original filename value will be set to temporary
>> snapshot if DCmdDump does not have recording name and filename.
>>
>> Also I added new testcase. It can test JFR.dump with filename.
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2019/04/23 6:48, Erik Gahlin wrote:
>>> I have looked further at the issue and while the message to the user 
>>> was incorrect previously ( "Dump failed. No data found in the 
>>> specified interval") it did result in a file with the specified 
>>> filename, i.e "test.jfr"
>>>
>>> When "clone.setDestination(this.destination);" is removed, the 
>>> filename passed on command line is ignored, but it does result in a 
>>> message that is consistent with what is happening (a file with an 
>>> autogenerated name is created). It does however change the behavior 
>>> of 'JFR.dump <pid> name=1'. Logic needs to be added to DCmdDump so 
>>> it uses an existing filename if it is set, but changes it if a user 
>>> is providing an override.
>>>
>>> When comes to testing. I think it would be better to write a 
>>> separate test that checks that things are dumped correctly 
>>> when-XX:StartFlightRecording:filename=<filename> is used.
>>>
>>> Thanks
>>> Erik
>>>
>>>> PING: Do you have any opinion(s) about this fix?
>>>>
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2019/04/01 21:46, Yasumasa Suenaga wrote:
>>>>> Hi,
>>>>>
>>>>> Do you have concern(s) about this change?
>>>>>
>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.01/
>>>>>
>>>>> As in the email below, I think it does not affect scenarios except 
>>>>> snapshot cloning.
>>>>> Please tell me if you have any opinions about this.
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2019/03/20 10:37, Yasumasa Suenaga wrote:
>>>>>> Hi Erik,
>>>>>>
>>>>>> I think this change does not affect other scenarios.
>>>>>>
>>>>>> :jdk_jfr jtreg tests and submit repo have passed with this change.
>>>>>> newSnapshotClone() is called by PlatformRecording::dump and
>>>>>> DCmdDump::newSnapshot.
>>>>>> Both methods are related to user-requested dump.
>>>>>>
>>>>>> What scenarios do you think? I want to check them.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>> 2019年3月19日(火) 23:59 Erik Gahlin <erik.gahlin at oracle.com>:
>>>>>>>
>>>>>>> Yes, but then you would lose the original destination in other 
>>>>>>> scenarios
>>>>>>>
>>>>>>> Maybe I can look at this next week.
>>>>>>>
>>>>>>> Erik
>>>>>>>
>>>>>>>> Hi Erik,
>>>>>>>>
>>>>>>>> Thank you for your reply.
>>>>>>>>
>>>>>>>> How about this changeset?
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.01/
>>>>>>>>
>>>>>>>> I think PlatformRecording#newSnapshotClone() should not dump.
>>>>>>>> So I removed `setDestination()` from this.
>>>>>>>>
>>>>>>>>
>>>>>>>> Yasumasa
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2019/03/19 22:04, Erik Gahlin wrote:
>>>>>>>>> This looks a bit hackish.
>>>>>>>>>
>>>>>>>>> The real issue seems to be that the method
>>>>>>>>> PlatformRecording#newSnapshotClone incorrectly sets the 
>>>>>>>>> destination in
>>>>>>>>> the clone, which then triggers a dump prematurely (when stop 
>>>>>>>>> is called).
>>>>>>>>>
>>>>>>>>> Purpose of that method is just to take a snapshot of existing 
>>>>>>>>> chunks in
>>>>>>>>> the disk repository (or from memory). and then let 
>>>>>>>>> dumpStopped(path) do
>>>>>>>>> the actual dump (copy out the data from the disk repository).
>>>>>>>>>
>>>>>>>>> To make it work, some other changes would be needed as well, 
>>>>>>>>> but I don't
>>>>>>>>> have time to investigate.
>>>>>>>>>
>>>>>>>>> Erik
>>>>>>>>>
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> Please review this change.
>>>>>>>>>>
>>>>>>>>>>      JBS: https://bugs.openjdk.java.net/browse/JDK-8220657
>>>>>>>>>>      webrev: 
>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.00/
>>>>>>>>>>
>>>>>>>>>> The user might not get flight record via JFR.dump if JFR is 
>>>>>>>>>> started
>>>>>>>>>> with filename option on target process.
>>>>>>>>>>
>>>>>>>>>> Please see JBS if you want to know how to reproduce.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>
>>>