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

Mon Jan 21 06:56:37 PST 2013

New webrev up at http://cr.openjdk.java.net/~ctornqvi/webrev/8006413/webrev.03/ , the changes from .02 are:

* Split stdout and stderr into their own buffers
* Added methods to OutputAnalyzer to analyze stdout/stderr separately (old method still looks at both of them)
* Closing the stdout/stderr streams in StreamPumper once we're done with them
* Added a OutputAnalyzerTest to test the OutputAnalyzer class

Kept the interrupted check based on David's comment.

Thanks,
Christian

-----Original Message-----
From: David Holmes 
Sent: den 21 januari 2013 03:13
To: Vladimir Ivanov
Cc: Christian Törnqvist; Yekaterina Kantserova; hotspot-dev at openjdk.java.net
Subject: Re: RFR (S): 8006413: Add utility classes for writing better multiprocess tests in jtreg

On 19/01/2013 12:18 AM, Vladimir Ivanov wrote:
> Christian,
>
> Nice work! Thanks for adding this.
>
> I have a couple of comments. Don't consider me too paranoic, please 
> :-)
>
> test/testlibrary/com/oracle/java/testlibrary/ProcessTools.java:
> 60 public static String getOutput(Process process) throws IOException 
> {
> 61 ByteArrayOutputStream buf = new ByteArrayOutputStream();
> 62 StreamPumper outPumper = new StreamPumper(process.getInputStream(),
> buf);
> 63 StreamPumper errPumper = new StreamPumper(process.getErrorStream(),
> buf);
>
> 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.

> 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 }
>
> 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]).

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