RFR 8066708: JMXStartStopTest fails to connect to port 38112

[Jaroslav Bachorik]

Hi Stuart,

On 15.1.2015 02:14, Stuart Marks wrote:
> On 1/9/15 4:17 AM, Jaroslav Bachorik wrote:
>> I've changed the PortAllocator to allocate an array of unique random
>> ports
>> instead of letting ServerSocket to take care of it.
>>
>> I ran the test 200x in a tight loop without a failure.
>>
>> I hope this will resolve this test's intermittent failures due to port
>> conflicts
>> once and for all.
>>
>> Update: http://cr.openjdk.java.net/~jbachorik/8066708/webrev.03
>
> Hi Jaroslav,
>
> Good to hear that the test seems to be running more reliably. (I'm
> assuming that you'd see failures before if you ran it 200x in a tight
> loop.) This is probably because you're avoiding the open/close/reopen
> approach that we've now thoroughly discredited. :-)
>
> That said, it still looks to me like the code is more complex than it
> needs to be. You're the one who's going to be maintaining it, but if it
> were me, I'd put more effort into simplifying it. Here are a few
> approaches I'd suggest.
>
> 1) It looks like occupyPort() is used only once. This is I think the
> only place in PortAllocator that actually opens a socket. It's used in
> test_09 where it looks like the intent is to keep a port busy by opening
> a socket, and then making sure that the JMX stuff in the child process
> does the right thing when it encounters a busy port.
>
> Since this is the only place that needs a real socket, you might as well
> move the ServerSocket creation stuff out of PortAllocator and create it
> directly here with new ServerSocket(0). This should never fail with a
> BindException, so you needn't worry about retries. Then get the local
> port, and pass it to the subprocess. You should put this within a
> try-with-resources in test_09 so that the socket will be closed properly.
>
> 2) If you do this, then PortAllocator gets a lot simpler. There's no
> need to keep a collection of sockets, so release() can go away, and the
> finally-block of withAllocatedPorts can go away too.
>
> 3) Now PortAllocator's only instance state is the array of random port
> numbers. But once this is generated, the only reason PortAllocator stays
> around is to host the getter for array elements; basically it's just a
> wrapper for the array. And the reset() method regenerates the array. The
> essence of this is now just a function that returns an array of N random
> port numbers. You can pass the array directly to the Task and it can
> just use the ports from the array, instead of calling a getter. If
> there's a BindException and a "reset" needs to be done, this is just
> another call to the function to generate another array. So there's
> really no longer a need to have PortAllocator instances.
>
> These is a bit farther afield from this particular change, but there are
> some other opportunities for simplification:
>
> 4) Each of the test_NN methods consists entirely of a println followed
> by a withAllocatedPorts() call which is passed a long multi-line lambda.
> (In my book, multi-line lambdas are a bit of a code smell.) The
> withAllocatedPorts() method essentially implements the
> retry-on-BindException policy. Since each test_NN method is invoked
> reflectively by a mini-framework in the test, you could merge that logic
> into the mini-framework. In turn, each test method would then call the
> random port generator method and get an array of the requested number of
> ports, and just use them. This would removing a level of nesting and a
> few lines of vertical space from every test method.
>
> 5) Most (but not all) of the tests call doSomething and then follow it
> with a try/finally block that calls stop(). It seems like this
> commonality could be extracted somehow, but it eludes me at the moment.
>
> 6) The internal jcmd() methods all have a void return value, but some of
> them take a Consumer, which is usually passed a lambda argument that
> performs some test and then sets an AtomicBoolean as a side effect.
> (This is another code smell. Sometimes it's necessary, but only
> sometimes.) This parameter really wants to be a Predicate. The jcmd()
> method can just call the predicate and keep track of the results, and
> return a final result boolean that, for example, indicates whether the
> predicate had ever returend true. I'm not sure that's exactly the
> semantic you want. But a preferable idiom is to return a value instead
> of calling a lambda that performs side effects on captured locals.
>

I've applied your comments and the code is a tad simpler now. I also 
ironed out a few more corner cases. I ran the test 500x in a tight loop 
and no failure, yay!

Update: http://cr.openjdk.java.net/~jbachorik/8066708/webrev.04

-JB-