Thanks for the feedback, good points! I've put an updated webrev here:
http://cr.openjdk.java.net/~allwin/8004183/webrev.01/ Changes: boolean instead of Boolean no longer sets worker as daemon Regards, /peter On Oct 11, 2013, at 1:13 PM, David Holmes <david.hol...@oracle.com> wrote: > On 11/10/2013 8:18 PM, Daniel Fuchs wrote: >> Hi Peter, >> >> Looks good to me - but you might want to use 'boolean' for >> isRunning rather than 'Boolean'. > > Definitely! > >> Joining on the daemon thread is probably not necessary, >> but there's no harm in it (the important part being >> isRunning=false + s.close()). > > It is pointless having the thread be a daemon now. If running in the same VM > as anything else joining will ensure this test is cleaned up before the next > test commences. > > David > ----- > >> best regards, >> >> -- daniel (not a reviewer) >> >> On 10/11/13 11:53 AM, shanliang wrote: >>> Looks good to me. >>> >>> Shanliang >>> >>> >>> Peter Allwin wrote: >>>> Hello! >>>> >>>> Looking for reviews of this fix where a jmxremote test left a worker >>>> thread running after completion. Fix is to flag the thread to finish >>>> and join before test method exit. >>>> >>>> bug: https://bugs.openjdk.java.net/browse/JDK-8004183 >>>> cr: http://cr.openjdk.java.net/~allwin/8004183/webrev.00 >>>> >>>> Thanks! >>>> /peter >>> >>
