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-
