From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3056A2BADB for ; Mon, 22 Oct 2018 00:21:34 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id iDZXiLYka_9L for ; Mon, 22 Oct 2018 00:21:34 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 411EE2BAD3 for ; Mon, 22 Oct 2018 00:21:33 -0400 (EDT) Date: Mon, 22 Oct 2018 07:21:30 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] jdbc: add connection timeout configuration and handling Message-ID: <20181022042130.4qf2lzj3pezfxi63@tkn_work_nb> References: <1539359249-27397-1-git-send-email-ztarvos@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1539359249-27397-1-git-send-email-ztarvos@gmail.com> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Sergei Kalashnikov Cc: tarantool-patches@freelists.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.