[tarantool-patches] Re: [PATCH] jdbc: add connection timeout configuration and handling

[Alexander Turenko <alexander.turenko@tarantool.org>]

Hi!

Thanks for your work!

It is really glad to see that you paying attention to cover your changes
and related code with tests.

I don't insist, but if it is not hard to do it would be good to have a
test that we'll get SQLException when a timeout exceeds on, say, trying
to get a table metadata or execute some statement. Now we test a timeout
only for the case when we try to connect.

Other comments are below.

WBR, Alexander Turenko.

> Added connection property `socketTimeout` to allow user control over
> network timeout before actual connection is returned by the driver.
> This is only done for default socket provider. The default timeout is
> is left to be infinite.

Typo: timeout is is -> timeout is.

> @@ -293,15 +299,28 @@ public class SQLConnection implements Connection {
>  
>      @Override
>      public void setNetworkTimeout(Executor executor, int milliseconds) throws SQLException {
> -        throw new SQLFeatureNotSupportedException();
> +        checkNotClosed();
> +
> +        if (milliseconds < 0)
> +            throw new SQLException("Network timeout cannot be negative.");
> +
> +        try {
> +            connection.setSocketTimeout(milliseconds);
> +        } catch (SocketException e) {
> +            throw new SQLException("Failed to set socket timeout: timeout=" + milliseconds, e);
> +        }
>      }

The executor is not used. Found [overview][1] of the problem. Checked
pgjdbc, they [do][2] the same. But mysql-connector-j [does not][3]. They
need to handle some complex cases like [4] (see also [5]) because of
that. To be honest I don't get Douglas's Surber explanation, but I think
we likely do right things here when just ignore the executor parameter.

[1]: http://mail.openjdk.java.net/pipermail/jdbc-spec-discuss/2017-November/000236.html
[2]: https://github.com/pgjdbc/pgjdbc/pull/849/files#diff-1ba924d72e9b18676e312b83bc90c7e7R1484
[3]: https://github.com/mysql/mysql-connector-j/tree/fe1903b1ecb4a96a917f7ed3190d80c049b1de29/src/com/mysql/jdbc/ConnectionImpl.java#L5495
[4]: https://bugs.mysql.com/bug.php?id=75615
[5]: https://github.com/mysql/mysql-connector-j/commit/e29f2e2aa579686e1e1549ac599e2f5e8488163b

> @@ -311,4 +330,28 @@ public class SQLConnection implements Connection {
>      public boolean isWrapperFor(Class<?> iface) throws SQLException {
>          throw new SQLFeatureNotSupportedException();
>      }
> +
> +    /**
> +     * @throws SQLException If connection is closed.
> +     */
> +    protected void checkNotClosed() throws SQLException {
> +        if (isClosed())
> +            throw new SQLException("Connection is closed.");
> +    }
> +
> +    /**
> +     * Inspects passed exception and closes the connection if appropriate.
> +     *
> +     * @param e Exception to process.
> +     */
> +    protected void handleException(Exception e) {
> +        if (CommunicationException.class.isAssignableFrom(e.getClass()) ||
> +            IOException.class.isAssignableFrom(e.getClass())) {
> +            try {
> +                close();
> +            } catch (SQLException ignored) {
> +                // No-op.
> +            }
> +        }
> +    }

Having the protected handleException method seems to break encapsulation
of the SQLConnection class (and maybe checkNotClosed too). I think it is
due to using the connection field (of the TarantoolConnection type)
outside of the class. Maybe we should wrap calls to connection.select
and so on with protected methods of the SQLConnection class like
nativeSelect and so on. And perform checkNotClosed and handleException
actions inside these wrappers. What do you think?

> @@ -45,22 +53,42 @@ public class SQLDriver implements Driver {
>          if (providerClassName != null) {
>              socket = getSocketFromProvider(uri, urlProperties, providerClassName);
>          } else {
> -            socket = createAndConnectDefaultSocket(urlProperties);
> +            // Passing the socket to allow unit tests to mock it.
> +            socket = connectAndSetupDefaultSocket(urlProperties, new Socket());
>          }
>          try {
> -            TarantoolConnection connection = new TarantoolConnection(urlProperties.getProperty(PROP_USER), urlProperties.getProperty(PROP_PASSWORD), socket) {{
> +            TarantoolConnection connection = new TarantoolConnection(
> +                urlProperties.getProperty(PROP_USER),
> +                urlProperties.getProperty(PROP_PASSWORD),
> +                socket) {{
>                  msgPackLite = SQLMsgPackLite.INSTANCE;
>              }};
>  
> -            return new SQLConnection(connection, url, info);
> -        } catch (IOException e) {
> -            throw new SQLException("Couldn't initiate connection. Provider class name is " + providerClassName, e);
> +            return new SQLConnection(connection, url, urlProperties);
> +        } catch (Exception e) {
> +            try {
> +                socket.close();
> +            } catch (IOException ignored) {
> +                // No-op.
> +            }
> +            throw new SQLException("Couldn't initiate connection using " + diagProperties(urlProperties), e);
>          }
> -
>      }

Are we really need to work with Socket and TarantoolConnection within
this class? Are we can create Socket inside TarantoolConnection and
TarantoolConnection inside SQLConnection? I think it will improve
encapsulation.

Hope we can mock it in some less intrusive way. Are we can?

Even if we'll need to pass some implementation explicitly via
constructors arguments (Socket for the TarantoolConnection constructor
and TarantoolConnection for the SQLConnection constructor), maybe it is
better to provide a class and not an instance to encapsulate handling of
possible construction exceptions inside TarantoolConnection /
SQLConnection implementations?

I don't very familiar with Java approaches (code patterns) to do such
things, so I ask you to help me find best approach here.

I don't push you to rewrite it right now (but maybe now is the good time
to do so), but want to consider alternatives and, then, either plan
future work or ask you to change things now (or, maybe, decide to leave
things as is).

>      @Override
>      public ResultSet executeQuery() throws SQLException {
> -        return new SQLResultSet(JDBCBridge.query(connection, sql, getParams()));
> +        connection.checkNotClosed();
> +        discardLastResults();
> +        Object[] args = getParams();
> +        try {
> +            return new SQLResultSet(JDBCBridge.query(connection.connection, sql, args));
> +        } catch (Exception e) {
> +            connection.handleException(e);
> +            throw new SQLException(formatError(sql, args), e);
> +        }
>      }
>  

The encapsulation concerns described above are applicable here too.

>      @Override
>      public int executeUpdate() throws SQLException {
> -        return JDBCBridge.update(connection, sql, getParams());
> -
> +        connection.checkNotClosed();
> +        discardLastResults();
> +        Object[] args = getParams();
> +        try {
> +            return JDBCBridge.update(connection.connection, sql, args);
> +        } catch (Exception e) {
> +            connection.handleException(e);
> +            throw new SQLException(formatError(sql, args), e);
> +        }
>      }
>  

The encapsulation concerns described above are applicable here too.

>      @Override
>      public boolean execute() throws SQLException {
> -        return false;
> +        connection.checkNotClosed();
> +        discardLastResults();
> +        Object[] args = getParams();
> +        try {
> +            return handleResult(JDBCBridge.execute(connection.connection, sql, args));
> +        } catch (Exception e) {
> +            connection.handleException(e);
> +            throw new SQLException(formatError(sql, args), e);
> +        }
>      }
>  

The encapsulation concerns described above are applicable here too.

I wonder also whether we can break things when call execute* methods in
parallel from multiple threads? Will one execute breaks resultSet for
the another? Of course it is not part of your issue, but maybe it should
be handled as a separate one.

>      @Override
>      public ResultSet executeQuery(String sql) throws SQLException {
> -        resultSet = new SQLResultSet(JDBCBridge.query(connection, sql));
> -        updateCount = -1;
> -        return resultSet;
> +        connection.checkNotClosed();
> +        discardLastResults();
> +        try {
> +            return new SQLResultSet(JDBCBridge.query(connection.connection, sql));
> +        } catch (Exception e) {
> +            connection.handleException(e);
> +            throw new SQLException("Failed to execute SQL: " + sql, e);
> +        }
>      }

The encapsulation concerns described above are applicable here too.

>      @Override
>      public int executeUpdate(String sql) throws SQLException {
> -        int update = JDBCBridge.update(connection, sql);
> -        resultSet = null;
> -        return update;
> +        connection.checkNotClosed();
> +        discardLastResults();
> +        try {
> +            return JDBCBridge.update(connection.connection, sql);
> +        } catch (Exception e) {
> +            connection.handleException(e);
> +            throw new SQLException("Failed to execute SQL: " + sql, e);
> +        }
>      }

The encapsulation concerns described above are applicable here too.

>      @Override
>      public boolean execute(String sql) throws SQLException {
> -        Object result = JDBCBridge.execute(connection, sql);
> -        if (result instanceof SQLResultSet) {
> -            resultSet = (SQLResultSet) result;
> -            resultSet.maxRows = maxRows;
> -            updateCount = -1;
> -            return true;
> -        } else {
> -            resultSet = null;
> -            updateCount = (Integer) result;
> -            return false;
> +        connection.checkNotClosed();
> +        discardLastResults();
> +        try {
> +            return handleResult(JDBCBridge.execute(connection.connection, sql));
> +        } catch (Exception e) {
> +            connection.handleException(e);
> +            throw new SQLException("Failed to execute SQL: " + sql, e);
>          }
>      }

The encapsulation concerns described above are applicable here too.

> +    @Test
> +    public void testNoResponseAfterInitialConnect() throws IOException {
> +        ServerSocket socket = new ServerSocket();
> +        socket.bind(null, 0);
> +        try {
> +            final String url = "tarantool://localhost:" + socket.getLocalPort();
> +            final Properties prop = new Properties();
> +            prop.setProperty(PROP_SOCKET_TIMEOUT, "3000");

Why so long? I think 100ms is enough. Tests will be run faster.