On Thu, 21 Nov 2024 09:26:19 GMT, Daniel Fuchs <dfu...@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
