RFR: 8344219: Remove calls to SecurityManager and doPrivileged in java.net.SocksSocketImpl after JEP 486 integration [v2]

[Daniel Fuchs]

On Thu, 21 Nov 2024 07:03:51 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> 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.

It would certainly be safer to preserve the original behaviour, also in case where getInputStream/getOutputStream might throw. I like the idea of doConnect, even if Volkan's original proposal had the advantage of getting rid of the cmdIn/cmdOut instance variables that don't seem to be needed in any other method than connect.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22271#discussion_r1851666018