You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2018/02/09 13:23:00 UTC
[GitHub] ivankelly closed pull request #1132: Fix shutdown race which left ZK session open
ivankelly closed pull request #1132: Fix shutdown race which left ZK session open
URL: https://github.com/apache/bookkeeper/pull/1132
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/AbstractLifecycleComponent.java b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/AbstractLifecycleComponent.java
index 38e73bf93..015d81c08 100644
--- a/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/AbstractLifecycleComponent.java
+++ b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/AbstractLifecycleComponent.java
@@ -64,12 +64,11 @@ public void removeLifecycleListener(LifecycleListener listener) {
@Override
public void start() {
- if (!lifecycle.canMoveToStarted()) {
+ if (!lifecycle.moveToStarted()) {
return;
}
listeners.forEach(LifecycleListener::beforeStart);
doStart();
- lifecycle.moveToStarted();
listeners.forEach(LifecycleListener::afterStart);
}
@@ -77,11 +76,10 @@ public void start() {
@Override
public void stop() {
- if (!lifecycle.canMoveToStopped()) {
+ if (!lifecycle.moveToStopped()) {
return;
}
listeners.forEach(LifecycleListener::beforeStop);
- lifecycle.moveToStopped();
doStop();
listeners.forEach(LifecycleListener::afterStop);
}
@@ -93,11 +91,10 @@ public void close() {
if (lifecycle.started()) {
stop();
}
- if (!lifecycle.canMoveToClosed()) {
+ if (!lifecycle.moveToClosed()) {
return;
}
listeners.forEach(LifecycleListener::beforeClose);
- lifecycle.moveToClosed();
try {
doClose();
} catch (IOException e) {
diff --git a/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/ComponentStarter.java b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/ComponentStarter.java
index 8ee6f77ff..fa9e9b1d4 100644
--- a/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/ComponentStarter.java
+++ b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/ComponentStarter.java
@@ -18,8 +18,9 @@
package org.apache.bookkeeper.common.component;
-import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.CompletableFuture;
import lombok.extern.slf4j.Slf4j;
+import org.apache.bookkeeper.common.concurrent.FutureUtils;
/**
* Utils to start components.
@@ -30,24 +31,25 @@
static class ComponentShutdownHook implements Runnable {
private final LifecycleComponent component;
- private final CountDownLatch aliveLatch;
+ private final CompletableFuture<Void> future;
ComponentShutdownHook(LifecycleComponent component,
- CountDownLatch aliveLatch) {
+ CompletableFuture<Void> future) {
this.component = component;
- this.aliveLatch = aliveLatch;
+ this.future = future;
}
@Override
public void run() {
- aliveLatch.countDown();
log.info("Closing component {} in shutdown hook.", component.getName());
try {
component.close();
log.info("Closed component {} in shutdown hook successfully. Exiting.", component.getName());
+ FutureUtils.complete(future, null);
} catch (Exception e) {
log.error("Failed to close component {} in shutdown hook gracefully, Exiting anyway",
component.getName(), e);
+ future.completeExceptionally(e);
}
}
@@ -58,14 +60,15 @@ public void run() {
*
* @param component component to start.
*/
- public static void startComponent(LifecycleComponent component,
- CountDownLatch aliveLatch) {
+ public static CompletableFuture<Void> startComponent(LifecycleComponent component) {
+ CompletableFuture<Void> future = new CompletableFuture<>();
Runtime.getRuntime().addShutdownHook(new Thread(
- new ComponentShutdownHook(component, aliveLatch), "component-shutdown-thread"));
+ new ComponentShutdownHook(component, future), "component-shutdown-thread"));
log.info("Starting component {}.", component.getName());
component.start();
log.info("Started component {}.", component.getName());
+ return future;
}
}
diff --git a/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/Lifecycle.java b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/Lifecycle.java
index 1ce06cfeb..1df689e69 100644
--- a/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/Lifecycle.java
+++ b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/Lifecycle.java
@@ -18,6 +18,8 @@
package org.apache.bookkeeper.common.component;
+import java.util.concurrent.atomic.AtomicReference;
+
/**
* Lifecycle state. Allows the following transitions:
*
@@ -67,65 +69,53 @@
CLOSED
}
- private volatile State state = State.INITIALIZED;
+ private AtomicReference<State> state = new AtomicReference<>(State.INITIALIZED);
public State state() {
- return this.state;
+ return this.state.get();
}
/**
* Returns <tt>true</tt> if the state is initialized.
*/
public boolean initialized() {
- return state == State.INITIALIZED;
+ return state.get() == State.INITIALIZED;
}
/**
* Returns <tt>true</tt> if the state is started.
*/
public boolean started() {
- return state == State.STARTED;
+ return state.get() == State.STARTED;
}
/**
* Returns <tt>true</tt> if the state is stopped.
*/
public boolean stopped() {
- return state == State.STOPPED;
+ return state.get() == State.STOPPED;
}
/**
* Returns <tt>true</tt> if the state is closed.
*/
public boolean closed() {
- return state == State.CLOSED;
+ return state.get() == State.CLOSED;
}
public boolean stoppedOrClosed() {
- Lifecycle.State state = this.state;
+ Lifecycle.State state = this.state.get();
return state == State.STOPPED || state == State.CLOSED;
}
- public boolean canMoveToStarted() throws IllegalStateException {
- State localState = this.state;
- if (localState == State.INITIALIZED || localState == State.STOPPED) {
- return true;
- }
- if (localState == State.STARTED) {
- return false;
- }
- if (localState == State.CLOSED) {
- throw new IllegalStateException("Can't move to started state when closed");
- }
- throw new IllegalStateException("Can't move to started with unknown state");
- }
-
-
public boolean moveToStarted() throws IllegalStateException {
- State localState = this.state;
- if (localState == State.INITIALIZED || localState == State.STOPPED) {
- state = State.STARTED;
- return true;
+ State localState = this.state.get();
+ while (localState == State.INITIALIZED || localState == State.STOPPED) {
+ if (state.compareAndSet(localState, State.STARTED)) {
+ return true;
+ } else {
+ localState = this.state.get();
+ }
}
if (localState == State.STARTED) {
return false;
@@ -136,56 +126,37 @@ public boolean moveToStarted() throws IllegalStateException {
throw new IllegalStateException("Can't move to started with unknown state");
}
- public boolean canMoveToStopped() throws IllegalStateException {
- State localState = state;
- if (localState == State.STARTED) {
- return true;
- }
- if (localState == State.INITIALIZED || localState == State.STOPPED) {
- return false;
- }
- if (localState == State.CLOSED) {
- throw new IllegalStateException("Can't move to stopped state when closed");
- }
- throw new IllegalStateException("Can't move to stopped with unknown state");
- }
-
public boolean moveToStopped() throws IllegalStateException {
- State localState = state;
- if (localState == State.STARTED) {
- state = State.STOPPED;
- return true;
- }
- if (localState == State.INITIALIZED || localState == State.STOPPED) {
+ State localState = state.get();
+ while (localState == State.STARTED) {
+ if (state.compareAndSet(localState, State.STOPPED)) {
+ return true;
+ } else {
+ localState = state.get();
+ }
+ }
+ if (localState == State.INITIALIZED
+ || localState == State.STOPPED
+ || localState == State.CLOSED) {
return false;
}
- if (localState == State.CLOSED) {
- throw new IllegalStateException("Can't move to stopped state when closed");
- }
throw new IllegalStateException("Can't move to stopped with unknown state");
}
- public boolean canMoveToClosed() throws IllegalStateException {
- State localState = state;
- if (localState == State.CLOSED) {
- return false;
- }
- if (localState == State.STARTED) {
- throw new IllegalStateException("Can't move to closed before moving to stopped mode");
- }
- return true;
- }
-
public boolean moveToClosed() throws IllegalStateException {
- State localState = state;
- if (localState == State.CLOSED) {
- return false;
- }
- if (localState == State.STARTED) {
- throw new IllegalStateException("Can't move to closed before moving to stopped mode");
+ State localState = state.get();
+ while (true) {
+ if (localState == State.CLOSED) {
+ return false;
+ } else if (localState == State.STARTED) {
+ throw new IllegalStateException("Can't move to closed before moving to stopped mode");
+ }
+ if (state.compareAndSet(localState, State.CLOSED)) {
+ return true;
+ } else {
+ localState = state.get();
+ }
}
- state = State.CLOSED;
- return true;
}
@Override
diff --git a/bookkeeper-common/src/test/java/org/apache/bookkeeper/common/component/TestComponentStarter.java b/bookkeeper-common/src/test/java/org/apache/bookkeeper/common/component/TestComponentStarter.java
index eadbbd945..e99c30124 100644
--- a/bookkeeper-common/src/test/java/org/apache/bookkeeper/common/component/TestComponentStarter.java
+++ b/bookkeeper-common/src/test/java/org/apache/bookkeeper/common/component/TestComponentStarter.java
@@ -22,7 +22,7 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
-import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.CompletableFuture;
import org.apache.bookkeeper.common.component.ComponentStarter.ComponentShutdownHook;
import org.junit.Test;
@@ -34,21 +34,20 @@
@Test
public void testStartComponent() {
LifecycleComponent component = mock(LifecycleComponent.class);
- CountDownLatch latch = new CountDownLatch(1);
when(component.getName()).thenReturn("test-start-component");
- ComponentStarter.startComponent(component, latch);
+ ComponentStarter.startComponent(component);
verify(component).start();
}
@Test
public void testComponentShutdownHook() throws Exception {
LifecycleComponent component = mock(LifecycleComponent.class);
- CountDownLatch latch = new CountDownLatch(1);
when(component.getName()).thenReturn("test-shutdown-hook");
- ComponentShutdownHook shutdownHook = new ComponentShutdownHook(component, latch);
+ CompletableFuture<Void> future = new CompletableFuture<>();
+ ComponentShutdownHook shutdownHook = new ComponentShutdownHook(component, future);
shutdownHook.run();
verify(component).close();
- latch.await();
+ future.get();
}
}
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieNettyServer.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieNettyServer.java
index 3c5385c5c..ea8f75089 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieNettyServer.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieNettyServer.java
@@ -402,6 +402,7 @@ void shutdown() {
try {
eventLoopGroup.shutdownGracefully(0, 10, TimeUnit.MILLISECONDS).await();
} catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
/// OK
}
}
diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/Main.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/Main.java
index c2f7b8e5c..292d535c4 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/Main.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/Main.java
@@ -25,7 +25,7 @@
import java.net.MalformedURLException;
import java.util.Arrays;
import java.util.List;
-import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
import lombok.extern.slf4j.Slf4j;
import org.apache.bookkeeper.bookie.ExitCode;
import org.apache.bookkeeper.common.component.ComponentStarter;
@@ -211,15 +211,15 @@ static int doMain(String[] args) {
}
// 2. start the server
- CountDownLatch aliveLatch = new CountDownLatch(1);
- ComponentStarter.startComponent(server, aliveLatch);
try {
- aliveLatch.await();
+ ComponentStarter.startComponent(server).get();
} catch (InterruptedException ie) {
// the server is interrupted
log.info("Bookie server is interrupted. Exiting ...");
+ } catch (ExecutionException ee) {
+ log.error("Error in bookie shutdown", ee);
+ return ExitCode.SERVER_EXCEPTION;
}
- server.close();
return ExitCode.OK;
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services