[ISSUE#4949] fix Address already in use

[kaori-seasons]

Make sure set the target branch to develop

What is the purpose of the change

Related to issue-4949

Fixed the port occupancy problem caused by the different order of shutdown calls

Verifying this change

Follow this checklist to help us incorporate your contribution quickly and easily. Notice, it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR.

• Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
• Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Write necessary unit-test(over 80% coverage) to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
• Run mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.
• If this contribution is large, please file an Apache Individual Contributor License Agreement.

[lizhanhui]

Hi @complone , nextPort() can only fix the issue when unit tests are running in sequential order. It would be better if we ask the OS to pick up an available port.
Aka, socket.listen(0).

[kaori-seasons]

细节

Fixed, re-apply for ports when applying for port occupancy from the server

But at the same time, I think the method marked @After will wait for all tests in the class to run before triggering, which will cause the port to be occupied, so I think it should be fixed here

[codecov-commenter]

Codecov Report

Merging #5000 (77b4471) into develop (4f0636a) will increase coverage by 0.08%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             develop    #5000      +/-   ##
=============================================
+ Coverage      43.22%   43.30%   +0.08%     
- Complexity      7724     7750      +26     
=============================================
  Files            995      995              
  Lines          69067    69126      +59     
  Branches        9143     9159      +16     
=============================================
+ Hits           29852    29935      +83     
+ Misses         35469    35434      -35     
- Partials        3746     3757      +11     

Impacted Files	Coverage Δ
...apache/rocketmq/broker/longpolling/PopRequest.java	31.03% <0.00%> (-13.80%)	⬇️
...a/org/apache/rocketmq/filter/util/BloomFilter.java	60.43% <0.00%> (-2.20%)	⬇️
.../apache/rocketmq/store/ha/DefaultHAConnection.java	65.86% <0.00%> (-1.21%)	⬇️
...ent/impl/consumer/DefaultLitePullConsumerImpl.java	69.37% <0.00%> (-0.63%)	⬇️
...he/rocketmq/controller/impl/DLedgerController.java	65.96% <0.00%> (-0.53%)	⬇️
...mq/store/ha/autoswitch/AutoSwitchHAConnection.java	71.50% <0.00%> (-0.27%)	⬇️
...org/apache/rocketmq/store/DefaultMessageStore.java	53.24% <0.00%> (-0.16%)	⬇️
...ain/java/org/apache/rocketmq/common/AclConfig.java	0.00% <0.00%> (ø)
.../org/apache/rocketmq/common/PlainAccessConfig.java	0.00% <0.00%> (ø)
...he/rocketmq/client/impl/consumer/ProcessQueue.java	62.38% <0.00%> (+0.45%)	⬆️
... and 10 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[aaron-ai]

细节

Hi, do you mean to apply for a free available port as follows:

            ServerSocket serverSocket = new ServerSocket(0);
            int port = serverSocket.getLocalPort();

Thank you for contributing!

It seems that the only way to find an available port is try to occupy a port using 0 and release it, but this approach is not thread-safe. You could get more information from the deprecation of SocketUtils#findAvailableTcpPort

[lizhanhui]

only way to find an available port is try to occupy a port using 0 and release it, but this approach is not thread-safe.

No, we should not use this approach. Modify the main source code to accept a configured listening port 0 and reflect/update the actual listening port in server-config objects. Use the config in test cases as we do in RemotingServerTest

[aaron-ai]

only way to find an available port is try to occupy a port using 0 and release it, but this approach is not thread-safe.

No, we should not use this approach. Modify the main source code to accept a configured listening port 0 and reflect/update the actual listening port in server-config objects. Use the config in test cases as we do in RemotingServerTest

I agree with that.

[kaori-seasons]

only way to find an available port is try to occupy a port using 0 and release it, but this approach is not thread-safe.

No, we should not use this approach. Modify the main source code to accept a configured listening port 0 and reflect/update the actual listening port in server-config objects. Use the config in test cases as we do in RemotingServerTest

cc @lizhanhui @aaron-ai I think such changes are in line with expectations. In fact, the default value of nettyServerConfig.getListenPort is 0. We do not need to implement the method for netty port compatibility in NettyRemotingServer.

[kaori-seasons]

cc @lizhanhui

As shown here, if the port of nettyserver is temporarily occupied by port 0, we need to add additional logic to release port 0 in NettyRemoteServer

https://github.com/complone/rocketmq/tree/fix-issue4949-port

But I am worried that this method is too invasive because we need to interact based on the Remoting protocol, which will cause the thread timing to be out of order.

[lizhanhui]

we need to add additional logic to release port 0 in NettyRemoteServer

Read https://www.baeldung.com/cs/binding-available-ports#different-ways-to-bind-a-port

But I am worried that this method is too invasive because we need to interact based on the Remoting protocol, which will cause the thread timing to be out of order.

Actually, this is the standard way to run tests in parallelism. I've spent some time investigating the potential changes required, finding we indeed need a few changes to the current code and dledger dependency. I am OK with the current status of this PR now that there is no achievable solution with changes less than 10 lines