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

[Vladimir Ivanov]

 >> 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.

 >> 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) {

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.com>
>>> 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
>>>