RFR (S): 8006413: Add utility classes for writing better multiprocess tests in jtreg
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Fri Jan 18 06:23:23 PST 2013
Hit "Send" too early...
One more comment: I don't see where you close streams acquired from
Process.getOutputStream()/Process.getErrorStream(). Is it overlooked?
Best regards,
Vladimir Ivanov
On 1/18/13 5:18 PM, 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.
>
> 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.
>
> 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
>>
More information about the hotspot-dev
mailing list