Potential infinite waiting at JMXConnection#createConnection

[Ben Evans]

Hi Kubota,

I think that perhaps the AdoptOpenJDK project
(https://java.net/projects/adoptopenjdk) can help here.

I've copied in their mailing list, in case one of the developers there
can help you.

Thanks,

Ben

On Mon, May 4, 2015 at 5:19 PM, KUBOTA Yuji <kubota.yuji at gmail.com> wrote:
> Hi all,
>
> I want to contribute this issue.
> If there are a problem about this patch or a better way for openjdk
> community, please advise me.
>
> Thanks for
>
> 2015-04-22 0:31 GMT+09:00 KUBOTA Yuji <kubota.yuji at gmail.com>:
>> Hi all,
>>
>> I found an infinite waiting at TCPChannel#createConnection.
>> This method flushes the DataOutputStream without the socket timeout settings
>> when choose stream protocol [1].
>>
>> If connection lost (the destination server do no return response)
>> during the flush,
>> this method has possibilities to take long time beyond the expectations
>> at java.net.SocketInputStream.socketRead0 as following stack trace.
>>
>> stack trace :
>>         at java.net.SocketInputStream.socketRead0(SocketInputStream.java)
>>         at java.net.SocketInputStream.read(SocketInputStream.java)
>>         at java.net.SocketInputStream.read(SocketInputStream.java)
>>         at sun.security.ssl.InputRecord.readFully(InputRecord.java)
>>         at sun.security.ssl.InputRecord.read(InputRecord.java)
>>         at sun.security.ssl.SSLSocketImpl.readRecord(SSLSocketImpl.java)
>>         at sun.security.ssl.SSLSocketImpl.performInitialHandshake(SSLSocketImpl.java)
>>         at sun.security.ssl.SSLSocketImpl.writeRecord(SSLSocketImpl.java)
>>         at sun.security.ssl.AppOutputStream.write(AppOutputStream.java)
>>         at java.io.BufferedOutputStream.flushBuffer(BufferedOutputStream.java)
>>         at java.io.BufferedOutputStream.flush(BufferedOutputStream.java)
>>         at java.io.DataOutputStream.flush(DataOutputStream.java)
>>         at sun.rmi.transport.tcp.TCPChannel.createConnection(TCPChannel.java)
>>         at sun.rmi.transport.tcp.TCPChannel.newConnection(TCPChannel.java)
>>         at sun.rmi.server.UnicastRef.invoke(UnicastRef.java)
>>         at javax.management.remote.rmi.RMIServerImpl_Stub.newClient
>>         at javax.management.remote.rmi.RMIConnector.getConnection(RMIConnector.java)
>>         at javax.management.remote.rmi.RMIConnector.connect(RMIConnector.java)
>>         at javax.management.remote.JMXConnectorFactory.connect(JMXConnectorFactory.java)
>>
>> When create connection, we cannot set the timeout by properties.
>> Therefore, JMX sets the default value of SO_TIMEOUT, i.e., infinite.
>> So I wrote a patch to fix this infinite waiting by using property-configured value:
>> sun.rmi.transport.tcp.responseTimeout.
>>
>> Please review this patch. :)
>>
>> Note: My OCA has been processed a few hour ago, so my name may take a
>> short time to
>> appear on the OCA signatories page.
>>
>> Thanks,
>> KUBOTA Yuji
>>
>> [1]: http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/c5b5d9045728/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPConnection.java#l191
>>
>> diff --git a/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
>> b/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
>> --- a/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
>> +++ b/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
>> @@ -222,20 +222,34 @@
>>                  // choose protocol (single op if not reusable socket)
>>                  if (!conn.isReusable()) {
>>                      out.writeByte(TransportConstants.SingleOpProtocol);
>>                  } else {
>>                      out.writeByte(TransportConstants.StreamProtocol);
>> +
>> +                    int usableSoTimeout = 0;
>> +                    try {
>> +                        /*
>> +                         * If socket factory had set a zero timeout on its own,
>> +                         * then set the property-configured value to prevent
>> +                         * an infinite waiting.
>> +                         */
>> +                        usableSoTimeout = sock.getSoTimeout();
>> +                        if (usableSoTimeout == 0) {
>> +                          usableSoTimeout = responseTimeout;
>> +                        }
>> +                        sock.setSoTimeout(usableSoTimeout);
>> +                    } catch (Exception e) {
>> +                        // if we fail to set this, ignore and proceed anyway
>> +                    }
>>                      out.flush();
>>
>>                      /*
>>                       * Set socket read timeout to configured value for JRMP
>>                       * connection handshake; this also serves to guard against
>>                       * non-JRMP servers that do not respond (see 4322806).
>>                       */
>> -                    int originalSoTimeout = 0;
>>                      try {
>> -                        originalSoTimeout = sock.getSoTimeout();
>>                          sock.setSoTimeout(handshakeTimeout);
>>                      } catch (Exception e) {
>>                          // if we fail to set this, ignore and proceed anyway
>>                      }
>>
>> @@ -279,18 +293,11 @@
>>                       * connection.  NOTE: this timeout, if configured to a
>>                       * finite duration, places an upper bound on the time
>>                       * that a remote method call is permitted to execute.
>>                       */
>>                      try {
>> -                        /*
>> -                         * If socket factory had set a non-zero timeout on its
>> -                         * own, then restore it instead of using the property-
>> -                         * configured value.
>> -                         */
>> -                        sock.setSoTimeout((originalSoTimeout != 0 ?
>> -                                           originalSoTimeout :
>> -                                           responseTimeout));
>> +                        sock.setSoTimeout(usableSoTimeout);
>>                      } catch (Exception e) {
>>                          // if we fail to set this, ignore and proceed anyway
>>                      }
>>
>>                      out.flush();