RFR 8066708: JMXStartStopTest fails to connect to port 38112

[Stuart Marks]

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.

-- 

This is quite a bit of stuff, and I don't necessarily expect you to fix it all. 
At least not in this changeset. But I've found that investing in refactoring of 
test code usually pays off, as it makes it easier to maintain the tests when 
you're forced to make changes to them again in six months.

s'marks