Skip to content

Speeding up the shutdown of the driver. #576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 29, 2019
Merged

Speeding up the shutdown of the driver. #576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0704e21
Speeding up the shutdown of the driver.
PetrJanouch Mar 14, 2019
f3312f9
Fix a flaky test
Apr 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion driver/src/main/java/org/neo4j/driver/internal/async/pool/ConnectionPoolImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.concurrent.CompletionStage;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentMap;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicBoolean;

Expand Down Expand Up @@ -165,7 +166,10 @@ public CompletionStage<Void> close()
}
finally
{
eventLoopGroup().shutdownGracefully();
// This is an attempt to speed up the shut down procedure of the driver
// Feel free return this back to shutdownGracefully() method with default values
// if this proves troublesome!!!
eventLoopGroup().shutdownGracefully(200, 15_000, TimeUnit.MILLISECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do? Add a timeout? What happens if the timeout expires... what is the user supposed to do?

I am very cautious about optimising for a very niche use case. We need to optimise for the general case, wherein the driver shuts down at application shutdown. Here, everything must close; there is no other code path.

Copy link
Contributor Author

@PetrJanouch PetrJanouch Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A short explanation how the shutdownGracefully works. It has 2 parameters ‘quiet period’ and ‘timeout’. The algorithm that it uses is the following:

shutdownStart = now()
while (true) {
	Thread.sleep(quietPeriod)
	if (nothingWasSubmittedToTheEventLoopWhileSleeping()) {
           break
	}

	if (now() - shutdownStart > timeout) {
          // timeout
	  break
        }
}

The main purpose of this shutdown algorithm is to compete work that is in progress.
The default value for ‘quiet period’ are 2 seconds and 15 seconds for ‘timeout’

What do we need a quiet period for anyway? The current shutdown logic is like this:

channels.each (ch -> closeAsync())
shutdownEventLoopWithQuietPeriod()

The graceful shutdown with the quiet period is needed because closing a channel async generates events submitted to the event loop.

What we would need is something like this:

channels.each (ch -> closeAsync())
waitForAllChannelsToBeClosed()
shutdownEventLoopWithoutQuietPeriod()

As simple as the pseudo code above looks, the problem is that achieving waitForAllChannelsToBeClosed() with current Netty API is not that simple (at least was not the last time I checked)

The problem is that the shutdown currently takes 4s (the quiet period is usually reset once by the tasks generated by closing the channels) which is enough to make for instance lambda users or users with a lot of integration tests sad.

The reasoning behind my PR was that the defaults in Netty are set so it would work with all possible use cases the toolkit is used for, but 200ms quiet period is more that enough for us.

}
}
return Futures.asCompletionStage( eventLoopGroup().terminationFuture() )
Expand Down
24 changes: 17 additions & 7 deletions examples/src/test/java/org/neo4j/docs/driver/ExamplesIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.Arrays.asList;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.emptyString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
Expand Down Expand Up @@ -462,20 +463,29 @@ void testSlf4jLogging() throws Exception
{
// log file is defined in logback-test.xml configuration file
Path logFile = Paths.get( "target", "test.log" );
Files.deleteIfExists( logFile );
if ( Files.exists( logFile ) )
{
// delete file made this test flaky
// erase content instead
Files.write( logFile, new byte[0] );
}

// verify erased
String logFileContent = new String( Files.readAllBytes( logFile ), UTF_8 );
assertThat( logFileContent, is( emptyString() ) );

String randomString = UUID.randomUUID().toString();
try ( Slf4jLoggingExample example = new Slf4jLoggingExample( uri, USER, PASSWORD ) )
{
String randomString = UUID.randomUUID().toString();
Object result = example.runReturnQuery( randomString );
assertEquals( randomString, result );
}
assertTrue( Files.exists( logFile ) );

assertTrue( Files.exists( logFile ) );
logFileContent = new String( Files.readAllBytes( logFile ), UTF_8 );
assertThat( logFileContent, containsString( "RETURN $x" ) );
assertThat( logFileContent, containsString( randomString ) );

String logFileContent = new String( Files.readAllBytes( logFile ), UTF_8 );
assertThat( logFileContent, containsString( "RETURN $x" ) );
assertThat( logFileContent, containsString( randomString ) );
}
}

@Test
Expand Down