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

[David Holmes]

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