AcceptorBindUtil - post feature fixes

[the-thing]

I noticed that one test is still failing

https://github.com/apache/mina/actions/runs/36934956436

Changes

• additional overloaded method in AcceptorBindUtil to try preferred port first if specified
• org.apache.mina.transport.socket.nio.SocketAcceptorTest#testBindTwice fix
• comments unified by copilot as this is the only thing I use AI for

[elecharny]

I'm a bit annoyed with the proposed patch... The idea was to be sure we can bind twice on the same address (host:port). Calling the tryBind() a second time will change the port, so it's not testing the same thing.

Here, what seems to happen is that even if the unbind() method is called, the socket remains in TIME_WAIT and can't be reused. It works just fine on mac, it fails on Linux.

I wonder if setting the SO_REUSEPORT wouldn't be a better solution.

[elecharny]

Nah, it's simpler that that. This part of the code recreate a socket address:

...
            int port = AcceptorBindUtil.tryBind(acceptor);
            InetSocketAddress address = new InetSocketAddress("127.0.0.1", port);
...

Changing the code to:

...
            int port = AcceptorBindUtil.tryBind(acceptor);
            InetSocketAddress address = acceptor.getLocalAddress() ;
...

fixes the test.
(I committed the code)

[the-thing]

Nothing to be annoyed about, that's what PR and reviews are for.

That is correct. So the idea of the test is to reuse the same pair "localhost" / port, but the port can be busy during the second attempt - that's what happened in the GH action. In the proposed patch we try to bind to the first one which can still fail, and if it does we fall back to the newly assigned port. So yes, at the end this is the "best effort" test and occasionally will attempt to bind to the same pair.

[the-thing]

Nah, it's simpler that that. This part of the code recreate a socket address:

...
            int port = AcceptorBindUtil.tryBind(acceptor);
            InetSocketAddress address = new InetSocketAddress("127.0.0.1", port);
...

Changing the code to:

...
            int port = AcceptorBindUtil.tryBind(acceptor);
            InetSocketAddress address = acceptor.getLocalAddress() ;
...

fixes the test. (I committed the code)

Ok. So you proposed patch is doing something else. You always try a new port. The question is what was the intention of the original test (no comments and badly named test method).

If reusing the same port is not crucial here than it is good patch.

[elecharny]

The original test was really testing we can re-bind after an unbind.
Changing the port is changing the test expected behavior IMO.

Side note for myself: test every patch on Mac AND Linux...

[elecharny]

FTR, there is something fishy.

The original code was:

        acceptor.setCloseOnDeactivation(false);
        acceptor.setReuseAddress(true);
        acceptor.setHandler(new IoHandlerAdapter());
        try {
            int port = AvailablePortFinder.getNextAvailable(1025);
            InetSocketAddress address = new InetSocketAddress("127.0.0.1", port);
            acceptor.bind(address);
            acceptor.unbind(address);
            acceptor.bind(address);
            acceptor.unbind(address);
        } finally {
            acceptor.dispose();
        }

I changed it to be:

        acceptor.setCloseOnDeactivation(false);
        acceptor.setReuseAddress(true);
        acceptor.setHandler(new IoHandlerAdapter());
        try {
            int port = AcceptorBindUtil.tryBind(acceptor);
            InetSocketAddress address = new InetSocketAddress("127.0.0.1", port);
            acceptor.unbind(address);
            acceptor.bind(address);
            acceptor.unbind(address);
        } finally {
            acceptor.dispose();
        }

The only difference is that the AcceptorBindUtil.tryBind(acceptor) call was binding the acceptor to an address that has nothing to do with InetSocketAddress("127.0.0.1", port) (actually, the used address was [0:0:0:0:0:0:0:0]:).

I suggest a modification of the AcceptorBindUtil.tryBind() method to add an optional host parameter, so the test would be:

        acceptor.setCloseOnDeactivation(false);
        acceptor.setReuseAddress(true);
        acceptor.setHandler(new IoHandlerAdapter());
        try {
            int port = AcceptorBindUtil.tryBind(acceptor, "127.0.0.1");
            InetSocketAddress address = acceptor.getLocalAddress();
            acceptor.unbind(address);
            acceptor.bind(address);
            acceptor.unbind(address);
        } finally {
            acceptor.dispose();
        }

wdyt?

[the-thing]

I suggest a modification of the AcceptorBindUtil.tryBind() method to add an optional host parameter, so the test would be:

Yes, this is a good change. I was planning to add it, but I didn't want to go crazy with changes to util in single PR.

[elecharny]

I'll push the change then.

[the-thing]

Closing down. This is obsolete now.

[the-thing]

Side note for myself: test every patch on Mac AND Linux...

Might be worth merging via PR to trigger actions.