RFR(XS): 8167001: [TESTBUG] java/lang/instrument/DaemonThread/TestDaemonThread.java fails when run by jprt

[serguei.spitsyn at oracle.com]

Ok, thanks.
Serguei


On 10/4/16 15:49, Chris Plummer wrote:
> I think in general that's the reason for using othervm. For this 
> particular test it's probably not needed since all the real work is 
> done by the creating subprocesses.
>
> Chris
>
> On 10/4/16 3:18 PM, serguei.spitsyn at oracle.com wrote:
>> I don't know the exact answer but I thought one motivation was
>> to avoid test/agent interference and so make it more stable.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 10/4/16 14:59, Chris Plummer wrote:
>>> Do you know why "othervm" is needed for the other j.l.i tests?
>>>
>>> Chris
>>>
>>> On 10/4/16 2:50 PM, serguei.spitsyn at oracle.com wrote:
>>>> Hi Chris,
>>>>
>>>> The createJavaProcessBuilder() approach is Ok with me.
>>>> Just curios why the "othervm" is not used for this case.
>>>> My theory is that it is because this test is located in sub-folder,
>>>> so that it was forgotten in some update. :)
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 10/4/16 14:33, Chris Plummer wrote:
>>>>> Hi Serguei,
>>>>>
>>>>> Yes, adding othervm also fixes the problem. What also works is 
>>>>> adding the following VM arguments when calling 
>>>>> createJavaProcessBuilder().:
>>>>>
>>>>>     "-cp", System.getProperty("test.class.path")
>>>>>
>>>>> or
>>>>>
>>>>>     "-cp", System.getProperty("java.class.path")
>>>>>
>>>>> I've seen the above used, othervm used, and relying on 
>>>>> ProcessTools.createJavaProcessBuilder() to add the classpath, 
>>>>> either implicitly as is done by default in hotspot/test, or 
>>>>> explicitly by passing true as the first arg as is done in jdk/test.
>>>>>
>>>>> My preferences is the createJavaProcessBuilder() approach. I worry 
>>>>> that othervm is relying on a side affect of its use, and is not on 
>>>>> a direct solution to the problem. I still haven't gotten an 
>>>>> explanation as to why othervm causes CLASSPATH to be exported, and 
>>>>> agentvm causes -classpath to be used instead. Using the 
>>>>> createJavaProcessBuilder() approach will continue to work even if 
>>>>> othervm stopped exporting CLASSPATH.
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>
>>>>> On 10/4/16 2:18 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Chris,
>>>>>>
>>>>>> It looks good to me.
>>>>>> But I have a question.
>>>>>> Would it also work if we add "othervm" to the test 
>>>>>> TestDaemonThread.java?
>>>>>> It seems other j.l.i tests are run in this mode.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>> On 10/4/16 10:43, Chris Plummer wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review the following:
>>>>>>>
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8167001
>>>>>>> http://cr.openjdk.java.net/~cjplummer/8167001/webrev.00/webrev.jdk/
>>>>>>>
>>>>>>> This fixes an issue with the classpath not being passed to the 
>>>>>>> subprocess, resulting in the main class for the subprocess not 
>>>>>>> being found. It only turns up when running the test with 
>>>>>>> "agentvm", which is the default when running the test via 
>>>>>>> test/Makefile (which is what jprt uses). "othervm" seems to be 
>>>>>>> the default when running jtreg directly from the command line, 
>>>>>>> so it doesn't see this problem.
>>>>>>>
>>>>>>> Although there are still ongoing discussion of jtreg classpath 
>>>>>>> definition differences between "agentvm" mode and "othervm" 
>>>>>>> mode, existing tests fix this issue by explicitly passing the 
>>>>>>> classpath to the subprocess. This is usually done by passing 
>>>>>>> "true" as the first argument to 
>>>>>>> ProcessTools.createJavaProcessBuilder(), so I'd like to go with 
>>>>>>> this fix for now. This is actually the default behavior for 
>>>>>>> hotspot/test. For jdk/test the default is "false" so you need to 
>>>>>>> explicitly pass "true".
>>>>>>>
>>>>>>> Tested by running with jprt under conditions where the test was 
>>>>>>> previously failing. Also tested by running the test directory 
>>>>>>> locally using jdk/Makefile (which previously was failing).
>>>>>>>
>>>>>>> thanks,
>>>>>>>
>>>>>>> Chris
>>>>>>
>>>>>
>>>>
>>>
>>
>