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

[Volkan Yazıcı]

On Thu, 21 Nov 2024 09:26:19 GMT, Daniel Fuchs <dfuchs at openjdk.org> wrote:

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

Agreed. Reverted to a synchronized method in 3b06bb14b653615d5095b2b20fbe672411f3e292. (Started a `tier3` run in parallel.)

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

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