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

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

On Thu, Nov 15, 2018 at 08:22:09PM +0300, Sergei Kalashnikov wrote:
> On Mon, Oct 22, 2018 at 07:21:30AM +0300, Alexander Turenko wrote:
> 
> Hi Alexander!
> 
> My sincere apologies for the slow reply; Please see my answers inline below.
> I also included the amended patch at the very end of this email.
> 
> Thank you.
> --
> Best regards,
> Sergei

Sorry for the slow reply too. It is hard for me to immerse enough to the
code to make a review useful...

I'm okay with tests and mostly okay with the patch. The most important
thing is that (I think) we should not break TarantoolConnection API.
Other notes are minor and are matters of opinion.

I think we should split features and refactoring in the future, because
it is hard to understand and review many different (not coupled)
changes. It is about future patches.

My comments are below.

WBR, Alexander Turenko.

> > > @@ -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.
> 
> My understanding is that executor in question was intended for providing a thread of
> execution that driver may use to track timeout, interrupt its own threads that perform
> network calls and cleanup the network resources afterwards. If the implementation can detect
> timeouts and cleanup resources without the use of provided executor, it may choose to do so.
> 
> In my opinion, the vague wording in specification alone is not a good reason to go and
> implement wrapping of a mere saving of timeout value into the executor just to add ourselves
> a concurrency issue and then add a test for it (it is what pg did).
> So this is not a right thing either.
> 

So we are on the one side. Great.

> > > @@ -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?
> > 
> 
> Yes, accesses like `connecttion.connection` must be cleaned up.
> I've changed the `connection` to private inside `SQLConnection` and made other
> refactorings including wrapping calls to TarantoolConnection methods
> outside of SQLConnection into new SQLConnection methods. But I tend to
> disagree regarding checkNotClosed(). It is useful by itself in situations
> when parameters passed to a function allow to return a local answer,
> but connection is closed.

Looks good.

> > > @@ -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).
> > 
> 
> I made an attempt to apply your proposition. Now a socket is created
> within TarantoolConnection which in its turn is created inside SQLConnection.
> The instantiations are done with the help of provider/factory interfaces.
> It doesn't look particularly well, I must admit, due to the fact that we
> need to provide a different interface on each nesting level and adapt them.
> I like the idea of SQLSocketProvider accepting an URI and properties, but
> the TarantoolConnection is unable to pass them by itself.
> 
> I guess we must think a little bit more on this.

I'm against of changing API of basic connector in incompatible manner.
TarantoolConnection constructor and even maybe TarantoolBase constructor
should not be changed. Can we add separate constructor with
SocketProvider? Or maybe create connected socket in SQLConnection's
constructor (yep, it is encapsulation break, but only in one place to
retain downside API -- should be properly commented I think).

To be honest I don't think that, say, providing
TarantoolConnectionFactory instead of TarantoolConnection in constructor
parameters improves encapsulation, because calling code still need to
create an object, which create TarantoolConnection in its method. I'm
understand the approach when it is optional parameter: a user who want
some custom behaviour able to provide it in that way, others don't
obligated to write 'boilerplace'. Other provider / fabric parameters
across the code are the same from this perspective.

Hovewer SQLConnection constructor is not part of public API, so I'm not
against this way if you found it convenient.

What would be ideal API from my point of view? Keeping in the mind
TarantoolConnection API should not be changed, the following:

```
class SQLConnection {
    SQLConnection(Properties properties) {
        String username = properties.getProperty(PROP_USER);
        String password = properties.getProperty(PROP_PASSWORD);
        try {
            Socket socket = getConnectedSocket(properties);
            this.connection = TarantoolConnection(username, password, socket);
        } catch(...) {
            // rethrow
        }
    }

    // Obtain connected socket to use as parameter for
    // TarantoolConnection.
    private Socket getConnectedSocket(Properties properties) {
        // Code from SQLSocketProviderImpl.getConnectedSocket().
    }
```

No need to introduce TarantoolConnectionFactory, SocketProvider[Impl],
SQLSocketProvider[Impl], SocketFoundry. Are there anything bad here that
I missed?

> > 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.
> > 
> 
> Yes, it will break.
> Let us address the aspects of multi-thread usage of a connection later on.
> (If that will ever be requested.)

I would prefer to formalize known issues even if we'll decide later to
don't fix them. Filed #95.

https://github.com/tarantool/tarantool-java/issues/95

>  @SuppressWarnings("Since15")
> -public class SQLConnection implements Connection {
> -    final TarantoolConnection connection;
> +public class SQLConnection implements Connection, SocketProvider {
> +    private final SQLSocketProvider sqlSocketProvider;
> +    private final TarantoolConnection connection;
>      final String url;
>      final Properties properties;
>  
> -    public SQLConnection(TarantoolConnection connection, String url, Properties properties) {
> -        this.connection = connection;
> +    SQLConnection(TarantoolConnectionFactory connectionFactory, SQLSocketProvider sqlSocketProvider, String url,
> +        Properties properties) throws SQLException {
> +        this.sqlSocketProvider = sqlSocketProvider;
>          this.url = url;
>          this.properties = properties;

Should we copy it to don't change external properties in case of
setClientInfo() call?

> +    protected int executeUpdate(String sql, Object ... args) throws SQLException {
> +        checkNotClosed();
> +        try {
> +            return JDBCBridge.update(connection, sql, args);
> +        } catch (Exception e) {
> +            handleException(e);
> +            throw new SQLException(formatError(sql, args), e);
> +        }
> +    }
> +

It seems that JDBCBridge is result formatter for SQL statements, but its
logic spread over multiple classes and is hard to understand, because of
that and because of naming. Also I found confusing that its methods get
TarantoolConnection as a parameter, but uses only results from last
operation.

It is not about your patch, just side note.

> +public class SQLSocketProviderImpl implements SQLSocketProvider {
> +    public final static SQLSocketProvider INSTANCE = new SQLSocketProviderImpl(new SocketFoundry() {
> +        public Socket makeSocket() {
> +            return new Socket();
> +        }
> +    });
> +
> +    private final SocketFoundry provider;
> +
> +    SQLSocketProviderImpl(SocketFoundry provider) {
> +        this.provider = provider;
> +    }
> +
> +    @Override
> +    public Socket getConnectedSocket(URI uri, Properties properties) {

uri is ignored, so this provider is very limited to be used in
SQLDriver. Such things should be commented.

> +        Socket socket = provider.makeSocket();

Cannot we just do `new Socket()` here? I don't insist at all, just
curious why such high level of abstraction was introduced.

> +    @Test
> +    public void testGetPropertyInfo() throws SQLException {
> +        Driver drv = new SQLDriver();
> +        Properties props = new Properties();
> +        DriverPropertyInfo[] info = drv.getPropertyInfo("tarantool://server.local:3302", props);
> +        assertNotNull(info);

I think it worth to add array size check here.