PING: RFR: 8220657: JFR.dump does not work when filename is set
Yasumasa Suenaga
yasuenag at gmail.com
Fri May 3 14:05:01 UTC 2019
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