On Wed, 20 Nov 2024 13:41:43 GMT, Volkan Yazıcı <d...@openjdk.org> wrote:
>> Removes `SecurityManager` et al. from `SocksSocketImpl`. `tier2` and `tier3`
>> tests have passed – CI run links are available in the ticket.
>
> Volkan Yazıcı has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update copyright year
src/java.base/share/classes/java/net/SocksSocketImpl.java line 288:
> 286: // Connects to the SOCKS server
> 287: try {
> 288: delegate.connect(new InetSocketAddress(server,
> serverPort), remainingMillis(deadlineMillis));
Hello Volkan, before this proposed change, the `privilegedConnect()` private
method used to be a `synchronized` method where it used to do the
`delegate.connect(...)` and then assign the input and output streams all as
part of that one synchronization. With the proposed change, the synchronization
is lost.
I think we should retain the previous behaviour. One way to do that would be to
introduce a private synchronized method like:
private synchronized void doConnect(final String host, final int port, final
int timeout)
throws IOException {
delegate.connect(new InetSocketAddress(host, port), timeout);
cmdIn = getInputStream();
cmdOut = getOutputStream();
}
and then call `doConnect(...)` from this line.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/22271#discussion_r1851467771
