PING: RFR: 8220657: JFR.dump does not work when filename is set
Yasumasa Suenaga
yasuenag at gmail.com
Mon May 13 10:21:07 UTC 2019
PING:
Could you review this webrev?
> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.03/
Yasumasa
On 2019/05/03 23:05, Yasumasa Suenaga wrote:
> Hi Erik,
>
> I uploaded new webrev:
> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.03/
>
> This webrev does not use `var` in any place.
> And I tweaked testcase based on your idea.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2019/05/02 23:41, Erik Gahlin wrote:
>> 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
>>>>>>>>>
>>>>>
>>
More information about the hotspot-jfr-dev
mailing list