You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "the-thing (via GitHub)" <gi...@apache.org> on 2026/10/02 05:57:41 UTC
[PR] AcceptorBindUtil - post feature fixes [mina]
the-thing opened a new pull request, #74:
URL: https://github.com/apache/mina/pull/74
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] AcceptorBindUtil - post feature fixes [mina]
Posted by "the-thing (via GitHub)" <gi...@apache.org>.
the-thing commented on PR #74:
URL: https://github.com/apache/mina/pull/74#issuecomment-5947655232
Closing down. This is obsolete now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] AcceptorBindUtil - post feature fixes [mina]
Posted by "elecharny (via GitHub)" <gi...@apache.org>.
elecharny commented on PR #74:
URL: https://github.com/apache/mina/pull/74#issuecomment-5947384436
I'll push the change then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] AcceptorBindUtil - post feature fixes [mina]
Posted by "elecharny (via GitHub)" <gi...@apache.org>.
elecharny commented on PR #74:
URL: https://github.com/apache/mina/pull/74#issuecomment-5947013439
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] AcceptorBindUtil - post feature fixes [mina]
Posted by "the-thing (via GitHub)" <gi...@apache.org>.
the-thing closed pull request #74: AcceptorBindUtil - post feature fixes
URL: https://github.com/apache/mina/pull/74
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] AcceptorBindUtil - post feature fixes [mina]
Posted by "the-thing (via GitHub)" <gi...@apache.org>.
the-thing commented on PR #74:
URL: https://github.com/apache/mina/pull/74#issuecomment-5947842704
> Side note for myself: test every patch on Mac _AND_ Linux...
Might be worth merging via PR to trigger actions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] AcceptorBindUtil - post feature fixes [mina]
Posted by "elecharny (via GitHub)" <gi...@apache.org>.
elecharny commented on PR #74:
URL: https://github.com/apache/mina/pull/74#issuecomment-5947233826
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]:<port>).
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] AcceptorBindUtil - post feature fixes [mina]
Posted by "elecharny (via GitHub)" <gi...@apache.org>.
elecharny commented on PR #74:
URL: https://github.com/apache/mina/pull/74#issuecomment-5946931684
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] AcceptorBindUtil - post feature fixes [mina]
Posted by "the-thing (via GitHub)" <gi...@apache.org>.
the-thing commented on PR #74:
URL: https://github.com/apache/mina/pull/74#issuecomment-5947042991
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] AcceptorBindUtil - post feature fixes [mina]
Posted by "the-thing (via GitHub)" <gi...@apache.org>.
the-thing commented on PR #74:
URL: https://github.com/apache/mina/pull/74#issuecomment-5947017348
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] AcceptorBindUtil - post feature fixes [mina]
Posted by "elecharny (via GitHub)" <gi...@apache.org>.
elecharny commented on PR #74:
URL: https://github.com/apache/mina/pull/74#issuecomment-5947081667
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...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] AcceptorBindUtil - post feature fixes [mina]
Posted by "the-thing (via GitHub)" <gi...@apache.org>.
the-thing commented on PR #74:
URL: https://github.com/apache/mina/pull/74#issuecomment-5947360035
> 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 one commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org