AbstractBindTest - retry available port bind

[the-thing]

Again a flaky test in 2.2.x branch.

https://github.com/apache/mina/actions/runs/35965588526/job/107523331814

In this case the issue is a time gap between org.apache.mina.util.AvailablePortFinder#getNextAvailable() and actually binding it with org.apache.mina.core.service.IoAcceptor#bind() since getNextAvailable can be made uavailable again.

The proposed change is to add a retry count in org.apache.mina.transport.AbstractBindTest#bind to try a few times. In my local environment it doesn't sometimes even happen for 1k test runs, but it happens more frequently with GitHub actions.

[the-thing]

Similar thing just happened in this PR's check for unrelated org.apache.mina.example.echoserver.ConnectorTest

https://github.com/apache/mina/actions/runs/36310962285/job/108596661471?pr=72

[elecharny]

Totally agree that we need to add a retry. As the test lay be run concurrently, there is no reason to expect that the AvailablePortFinder returns a port that will be available when we try to use it.

We can't guarantee that this method will return an available port, it's a "best effort" situation.

[elecharny]

Note that this method is used in 18 tests...

[the-thing]

I have to think what to with this. Either we explicitly use anonymous bind (I guess some tests do not do it intentionally) or just write some common code for all of them where possible.

[elecharny]

May be using a method like:

public class AcceptorBindUtil {
    public static final int tryBind(IoAcceptor acceptor) {
        int nbTry = 10;
        
        while (true) {
            int nextAvailable = AvailablePortFinder.getNextAvailable();
            try {
                acceptor.bind( new InetSocketAddress( nextAvailable ) );
                
                return nextAvailable;
            } catch ( IOException e ) {
                nbTry--;
                
                if (nbTry == 0) {
                    throw new RuntimeException(e);
                }
            }
        }
    }
}

which is called this way;

                    int nextAvailable = AcceptorBindUtil.tryBind( acceptor );

could do the trick (possibly passing the number of tries as a parameter, too)

[elecharny]

I'll push the suggested class and the modified test as soon as my daughter is on her way to school ;-)

(and of course if the tests pass...)

[elecharny]

I have the tests passing with half the tests fixed, keep going

[the-thing]

Are you pushing the changes somewhere? If you get bored / tired I can continue off where you started.

[elecharny]

I'm half done, but being busy on Apache LDAP API/Apache Directory Studio release those days, some CVAs and day job. Will try to catch up tonite.

[elecharny]

Still some issues with the AbstractBindtest. I have it working fine for Socket, not for VmPipe.

Working on it...

[the-thing]

Cool. I have a prototype patch for one of reported concurrency issues, but it will require the bind common code to make the test more resilient. I will wait.

[elecharny]

The difficulty with AbstractBindTest is that it call the createSocketAddress class from one of it's inherited classes, and it requires a port, which has not yet be selected, because we want to do that in the AcceptorBindUtil.tryBind call.
And when the Acceptor is bound we can't anymore store the LocalAddress into it.

A solution would be to remove the AbstractBindTest class and move its code in the three test classes that inherit from it. A bit heavy but it would give the opportinity to properly handle the localAddress accordingy to the acceptor type.

[elecharny]

I think I found a better solution for AbstractBindTest, and now the associated tests are passing.

Still fighting with some other test failures...

[elecharny]

Nice, I have the tests passing now.

Still a few places to change to get rid of the AvailablePortFinding() method.

[elecharny]

Get all test passing green now.

I have pushed my changes, please feel free to pull the code and test it.

[the-thing]

I will have a look. Let me close this one as it is probably obsolete.