RFR (S): 8006413: Add utility classes for writing better multiprocess tests in jtreg

Wed Jan 23 04:37:30 PST 2013

I've changed the order of the Thread.interrupted check as suggested by David Holmes and Vladimir Ivanov.

As for leaving the stdout/stderr interleaved, that is something that can easily be addressed later if there is a need for it.

Thanks for all the great reviews, list of reviewers (if I've missed anyone, please let me know): brutisso, vitalyd at gmail.com, dholmes, vlivanov, nloodin, mgerdin 

Thanks,
Christian

-----Original Message-----
From: David Holmes 
Sent: den 22 januari 2013 00:59
To: Vladimir Ivanov
Cc: hotspot-dev at openjdk.java.net
Subject: Re: RFR (S): 8006413: Add utility classes for writing better multiprocess tests in jtreg

On 22/01/2013 4:59 AM, Vladimir Ivanov wrote:
>
>  >> stdout & stderr are stored in the same buffer. I don't see why the
>>> output can't be interleaved, so consequent analysis could fail 
>>> intermittently. I suggest to store and analyze them separately. 
>>> Also, it'll allow to limit the search to concrete stream.
>>
>> So one problem here, regardless of whether you use one buf or two is 
>> that there seems to be no way to correlate the output and error 
>> streams, unless they were already going to the same sink - in which 
>> case you would want them both to go to buf anyway.
> I don't think that correlating streams' content is a requirement for 
> proposed solution.
> Otherwise, more complex approach should be used here.

I would always want stdout and stderr combined otherwise you can't tell how they relate.

>  >> Thread.interrupted call is redundant: if the thread is 
> interrupted,  >> exception will be thrown from localIn.read call anyway.
>  >
>  > That is dependent on the underlying stream implementation and 
> whether  > interruptible I/O is supported (which it no longer is [and 
> only ever was  > on Solaris]).
> Thanks, I wasn't aware about that.
>
> Shouldn't the interruption status check be reordered with read then?
>  >> 60 while (!Thread.interrupted() && (length = localIn.read(buffer)) 
> >
> 0) {

It would be slightly better that way:
- quick exit if interrupted just after start
- tries to write out whatever was read in (assuming the read/write don't throw InterruptedIOException)

David
-----

> Best regards,
> Vladimir Ivanov
>
>>> In the following code:
>>> test/testlibrary/com/oracle/java/testlibrary/StreamPumper.java:
>>> 54 public void run() {
>>> 55 int length;
>>> 56 try {
>>> 57 byte[] buffer = new byte[BUF_SIZE];
>>> 58 InputStream localIn = in;
>>> 59 OutputStream localOut = out;
>>> 60 while ((length = localIn.read(buffer)) > 0 &&
>>> !Thread.interrupted()) {
>>> 61 localOut.write(buffer, 0, length);
>>> 62 }
>>> 63 localOut.flush();
>>> 64 } catch (IOException e) {
>>> 65 // Just abort if something like this happens.
>>> 66 e.printStackTrace();
>>> 67 }
>>> 68 }
>>>
>>
>> David
>> -----
>>
>>> Also, I would suggest to move localOut.flush to finally clause, 
>>> since the call can be skipped in case of thread interruption.
>>>
>>> Best regards,
>>> Vladimir Ivanov
>>>
>>> On 1/18/13 3:43 PM, Christian Törnqvist wrote:
>>>> Another updated webrev at
>>>> http://cr.openjdk.java.net/~ctornqvi/webrev/8006413/webrev.02/ , 
>>>> formatting changes.
>>>>
>>>> Thanks,
>>>> Christian
>>>>
>>>> -----Original Message-----
>>>> From: Christian Törnqvist
>>>> Sent: den 17 januari 2013 17:44
>>>> To: hotspot-dev at openjdk.java.net
>>>> Cc: Vitaly Davidovich; Yekaterina Kantserova
>>>> Subject: RE: RFR (S): 8006413: Add utility classes for writing 
>>>> better multiprocess tests in jtreg
>>>>
>>>> Updated webrev can be found at
>>>> http://cr.openjdk.java.net/~mgerdin/ctornqvi/8006413/webrev.01/ . 
>>>> Lots of small changes based on all the great feedback I got :)
>>>>
>>>> Thanks,
>>>> Christian
>>>>
>>>> -----Original Message-----
>>>> From: Christian Törnqvist
>>>> Sent: den 17 januari 2013 15:19
>>>> To: Vitaly Davidovich
>>>> Cc: hotspot-dev at openjdk.java.net
>>>> Subject: RE: RFR (S): 8006413: Add utility classes for writing 
>>>> better multiprocess tests in jtreg
>>>>
>>>> Hi Vitaly,
>>>>
>>>> First of all, thanks for all your feedback :)
>>>>
>>>>> - not entirely sure what getPlatformSpecificVMArgs is really meant 
>>>>> for. Maybe just have an is64bitJVM() instead? You can just return 
>>>>> "new String[0]" though (I'm not a fan of extra >braces)
>>>>
>>>> This is for dealing with platform specific arguments to the java 
>>>> launcher (more importantly, not having the test worry about these 
>>>> things). Right now we only have the requirement to send '-d64' to 
>>>> any process launched as 64bit on Solaris, in the future we might 
>>>> have other requirements. That's the reason for giving this a more 
>>>> general name.
>>>>
>>>>> - there's no non-reflective way to get the pid?
>>>>
>>>> Not that I've found :(
>>>>
>>>>
>>>>
>>>> I've taken a lot of your comments and made changes to the classes, 
>>>> I'll be publishing an updated webrev soon.
>>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Christian
>>>>
>>>>
>>>>
>>>> From: Vitaly Davidovich [mailto:vitalyd at gmail.com]
>>>> Sent: den 16 januari 2013 16:30
>>>> To: Christian Törnqvist
>>>> Cc: hotspot-dev at openjdk.java.net
>>>> Subject: RE: RFR (S): 8006413: Add utility classes for writing 
>>>> better multiprocess tests in jtreg
>>>>
>>>>
>>>>
>>>> Some of this is style so feel free to ignore:
>>>>
>>>> I'd mark all these classes final for now. I'd also hide the default 
>>>> ctor for the util classes that only have static methods.
>>>>
>>>> JDKToolFinder
>>>> - Check that the jdk.test sysprop is not empty as well.
>>>>
>>>> - Remove the throws clause from the method - don't think it adds 
>>>> any value.
>>>>
>>>> - RuntimeException seems more idiomatic instead of Exception
>>>>
>>>> - after creating the full path string, might be nice to verify that 
>>>> it exists and throw if not (things will not work later but you can 
>>>> provide a better error message here). Perhaps even return a File or 
>>>> Path instance instead of string.
>>>>
>>>> OutputAnalyzer
>>>> - any reason fields are default visible? Make them private and final.
>>>>
>>>> - should probably check that output is not null in the constructors
>>>>
>>>> - it's possible to chain the constructors here
>>>>
>>>> - same comment about throws clauses on methods + RuntimeException 
>>>> as above
>>>>
>>>> ProcessTools
>>>> - getOutput swallows IOException and returns "". Not sure this is 
>>>> best
>>>> - I think the test should fail as you can't trust the results at 
>>>> this point.
>>>>
>>>> - StreamPumper extends Thread so I don't think you need the wrapper 
>>>> threads here. Cleaner is to make StreamPumper implement Runnable 
>>>> though and use threads here.
>>>>
>>>> - interruption is caught and restored but no guarantee caller will 
>>>> observe it now. Again, don't think returning "" is right as results 
>>>> are indeterminate. I'd let the interrupt bubble up.
>>>>
>>>> - there's no non-reflective way to get the pid?
>>>>
>>>> - not entirely sure what getPlatformSpecificVMArgs is really meant 
>>>> for. Maybe just have an is64bitJVM() instead? You can just return 
>>>> "new String[0]" though (I'm not a fan of extra braces)
>>>>
>>>> -createJavaProcessBuilder() can probably just use
>>>> List.addAll(Arrays.asList(...)) but Collections. addAll is OK too.
>>>>
>>>> StreamPumper
>>>> - check ctor args for null reference. The fields can be final as well.
>>>>
>>>> That's all for now :).
>>>>
>>>> Thanks
>>>> Sent from my phone
>>>>
>>>> On Jan 16, 2013 9:42 AM, "Christian Törnqvist" <HYPERLINK 
>>>> "mailto:christian.tornqvist at oracle.com"christian.tornqvist at oracle.c
>>>> om>
>>>> wrote:
>>>>
>>>> Hi Vitaly,
>>>>
>>>>
>>>>
>>>> Setting them as daemon sounds like a good idea :)
>>>>
>>>>
>>>>
>>>> I'd like your feedback on the other things as well, the quality of 
>>>> test code is important.
>>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Christian
>>>>
>>>>
>>>>
>>>> From: Vitaly Davidovich [mailto:HYPERLINK "mailto:vitalyd at gmail.com"
>>>> \nvitalyd at gmail.com]
>>>> Sent: den 16 januari 2013 14:50
>>>> To: Christian Törnqvist
>>>> Cc: HYPERLINK "mailto:hotspot-dev at openjdk.java.net"
>>>> \nhotspot-dev at openjdk.java.net
>>>> Subject: Re: RFR (S): 8006413: Add utility classes for writing 
>>>> better multiprocess tests in jtreg
>>>>
>>>>
>>>>
>>>> Hi Christian,
>>>>
>>>> Not sure how polished you want/need this code to be since it's just 
>>>> utility, but one thing that jumped out was the stream pumper 
>>>> threads for reading stdout/err should be daemonized just to play it safe.
>>>>
>>>> There are some other things that can be done, but not sure it's 
>>>> worth the effort for a testing utility. Let me know though if you'd 
>>>> like to cover everything.
>>>>
>>>> Thanks
>>>>
>>>> Sent from my phone
>>>>
>>>> On Jan 16, 2013 7:36 AM, "Christian Törnqvist" <HYPERLINK 
>>>> "mailto:christian.tornqvist at oracle.com"
>>>> \nchristian.tornqvist at oracle.com> wrote:
>>>>
>>>> Hi everyone,
>>>>
>>>>
>>>>
>>>> This RFE adds a few utility classes to make it a bit easier to 
>>>> write multi-process tests in jtreg, webrev can be found at 
>>>> http://cr.openjdk.java.net/~brutisso/8006413/webrev.00/
>>>>
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Christian
>>>>