question about InternalSession.isOpen method

[inserpio]

Hi All, I'm Lorenzo from the Neo4j JDBC driver team.

I'm contacting you because we have this open issue 69 that looks like to be somehow related to your InternalSession.isOpen method.
In fact, the guy that opened this issue is also proposing to slightly change this your method to solve his problem (please see: issue 69 comment).

Design choice for the JDBC Driver's BOLT implementation was to wrap as much as possible this your java driver (carefully monitoring the overhead we're introducing not to lower performances): that's why we'd like to keep calling your methods instead of rewriting the same logic of you.

So, what's your opinion about this our issue? Do you think this is something we'd solve from our side or the proposed patch sounds reasonable for you? Or whatelse?

Cheers,
Lorenzo

[zhenlineo]

We actually made the connection.isOpen and session.isOpen differently on purpose. connection.isOpen test if the socket connection with a server has been closed, whereas session.isOpen test if the session has free the connection to so-called connection pool.

In our bolt driver design, a connection refers to a real socket connection between a driver and a server, while a session is a logical idea of a "conversation" with the server. We expect a user to run a cypher statement and then "close" this "conversation" (session). When the user closes this session, we do not close the connection but return it in the connection pool so that we could reuse it latter. Therefore we expect a connection will not be closed before a session.

By reading the original issue, I was wondering why the connection is closed before the session. In our driver code, the only way that the connection is closed before the session is that we received a fatal bolt protocol error message. So it might because a bug in our code that result in the user having the problems.

Could you ask the user if he has received some exception before he hits this connection already closed error?

Thanks in advance.
Zhen

[inserpio]

Thanks for the reply @zhenlineo and for the useful clarification about the driver design.
I'll ask some more info to that guy and ping you back.
Cheers,
Lorenzo

[troyplay]

We also found the same issue when interrupting the thread of JDBC connection communicating with the server. And that's properly because of some kind of client interrupted exception thrown by socket channel which makes session state incompatible with connection state. Looking forward to more advices and best practices.

[zhenlineo]

@troyplay Thanks very much for sharing your experience. May I ask why you want to interrupt the the JDBC connection? (Maybe it is a fancy feature in JDBC driver?)

If I understand correctly, this JDBC connection holds a bolt session in its underlying impl. So interrupting the JDBC connection will be interrupting the communication between server and client. I would then worrying if this would result in half server/client message received/sent. Neither the server or the client could handle half message and they would consider this half message as a fatalProtocolError. As a result, we would see this "connection closed before session" error.

cc @inserpio If this interrupting is a feature JDBC driver provides, could you get some tests to verify that we will give a user a new bolt session if they want to carry on after interruption in the same JDBC connection?

[inserpio]

Hi @zhenlineo, no we don't have any kind of interrupting feature.
JDBC interface declares a setQueryTimeout method, but we don't manage it yet.
I'm checking if for some reason (i.e. when an exception occurs) we accidentally corrupt the bolt session we wrap.

[rubby33]

Similarly, In our system, the user needs to get a reply in 1 second. If the thread that execute neo4j query time is more than 1 second, we will interrupt the thread.

[troyplay]

@zhenlineo I guess it's quite possible for users to cancel the task or interrupt the thread in real cases especially when query is blocked. And I wonder whether it would be better for session to handle the thread interrupted status before receiving/sending message. Thanks.

[zhenlineo]

@troyplay @rubby33 I believe what we are missing here is a function in bolt driver to stop a long running query. Using thread interrupt is not an ideal way to do it. We DO have a more proper way to kill a long running query, which have not yet in place. We will explore on killing queries but meanwhile, when you interrupted a bolt driver session, do expect the socket connection to be broken afterwards. Using a new session afterwards will probably let this issue go away.

[troyplay]

@zhenlineo Thanks a lot for your guide.

[inserpio]

Hi @zhenlineo, I just added this test on the JDBC driver. It kills the thread while it's executing a query and when I try to reuse the connection for another query (outside that thread) the wrapped bolt session is indeed corrupted.

Here is the exception stacktrace:

Exception in thread "Thread-4" org.neo4j.driver.v1.exceptions.ClientException: The current session cannot be reused as the underlying connection with the server has been closed due to unrecoverable errors. Please close this session and retry your statement in another new session.
    at org.neo4j.driver.internal.InternalSession.ensureConnectionIsOpen(InternalSession.java:205)
    at org.neo4j.driver.internal.InternalSession.ensureConnectionIsValidBeforeRunningSession(InternalSession.java:162)
    at org.neo4j.driver.internal.InternalSession.run(InternalSession.java:92)
    at org.neo4j.driver.internal.InternalSession.run(InternalSession.java:86)
    at org.neo4j.driver.internal.InternalSession.run(InternalSession.java:66)
    at org.neo4j.jdbc.bolt.BoltConnection$1.run(BoltConnection.java:225)

From our side we'll probably change the code to mark the connection as invalid, but I need to discuss that with the team.

[inserpio]

@zhenlineo I'm observing that the test assertFalse(connection.isClosed()) I execute right after the thread has been interrupted, is returning false, which means that the connection looks to be still ok according to the jdbc connection.

Now, our isClosed method just recalls your session.isOpen() and this your method is returning true meaning that the state variable isOpen is still set to trueeven if the socket is broken.

This last thing finally makes me loop back to the first post of this conversation and the proposed solution posted by @rubby33 here: neo4j/neo4j-jdbc#69 (comment)

On the contrary, we can of course change our implementation of isClose so to return true when we in some ways detect that the socket is closed.

[zhenlineo]

@inserpio If you want to handle this interrupt problem for users, then you might try to use reflection to get this private method internalSession.ensureConnectionIsValidBeforeRunningSession or internalSession.ensureConnectionIsValidBeforeOpeningTransaction (The only diff is the error message, so either of them will work) to test if a session is valid. However as the method will throw exception, so it is a expensive check.

I also wondering if we should handle this error for users, as if we interrupt, then we should expect all IO and buffers would become corrupted.

[zhenlineo]

@troyplay @rubby33 A better way to kill a session would be provided in 1.1 after it is fully released: #210
Example of use: code

[zhenlineo]

We have released a new milestone version 1.1.0-M02 which include the new session#reset method. Please be free to try out this new approach of killing long running statements.
Example of use:

        try( final Session session = driver.session() )
        {
            StatementResult result = session.run( "CALL test.driver.longRunningStatement({seconds})",
                    parameters( "seconds", 30 ) );

            resetSessionAfterTimeout( session, 5 );

            while( result.hasNext() )
            {
                result.next(); // do something with the result
            }

            fail("Should have got an exception about streaming get killed.");
        }
        catch( ClientException e )
        {
            assertThat( e.getMessage(), containsString( "The transaction has been terminated" ) );
        }

where resetSessionAfterTimeout method could be something like

    private void resetSessionAfterTimeout( final Session session, final int timeout )
    {
        new Thread( new Runnable()
        {
            @Override
            public void run()
            {
                try
                {
                    Thread.sleep( timeout * 1000 ); // let the statement executing for timeout seconds
                }
                catch ( InterruptedException e )
                {
                    e.printStackTrace();
                }
                finally
                {
                    session.reset(); // reset the session after timeout
                }
            }
        } ).start();
    }

@inserpio I hope with this new killing statement approach, users would be free of Java thread interrupt and we will not see more connection closed exception. Is this answer good enough for you to close this issue? Or please let up know if you have more thinkings on this issue.