On Thu, 21 Nov 2024 07:03:51 GMT, Jaikiran Pai <j...@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
