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
