Skip to content
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.

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

Avoid race when shutting down controller processes #24579

Merged

Conversation

@droberts195
Copy link
Contributor

commented May 10, 2017

This commit terminates any controller processes plugins might have after
the node has been closed. This gives the plugins a chance to shut down their
controllers gracefully.

Previously there was a race condition where controller processes could be shut
down gracefully and terminated by two threads running in parallel, leading to
non-deterministic outcomes.

Additionally, controller processes that failed to shut down gracefully were
not forcibly terminated when running as a Windows service; there was a reliance
on the plugin to shut down its controller gracefully in this situation.
This commit also fixes this problem.

Avoid race when shutting down controller processes
This commit terminates any controller processes plugins might have after
the node has been closed.  This gives the plugins a chance to shut down their
controllers gracefully.

Previously there was a race condition where controller processes could be shut
down gracefully and terminated by two threads running in parallel, leading to
non-deterministic outcomes.

Additionally, controller processes that failed to shut down gracefully were
not forcibly terminated when running as a Windows service; there was a reliance
on the plugin to shut down its controller gracefully in this situation.
This commit also fixes this problem.
@jasontedor
Copy link
Member

left a comment

@droberts195 I left a suggestion.

@@ -190,7 +180,7 @@ public void run() {
Runtime.getRuntime().addShutdownHook(new Thread() {
@Override
public void run() {
try {
try (Spawner spawnerToClose = spawner) {

This comment has been minimized.

Copy link
@jasontedor

jasontedor May 10, 2017

Member

We should keep logging as the last component to shutdown, maybe we should do this:

diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java
index 3bfc2ac3b5..1e53faa9ef 100644
--- a/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java
+++ b/core/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java
@@ -180,8 +180,8 @@ final class Bootstrap {
             Runtime.getRuntime().addShutdownHook(new Thread() {
                 @Override
                 public void run() {
-                    try (Spawner spawnerToClose = spawner) {
-                        IOUtils.close(node);
+                    try {
+                        IOUtils.close(node, spawner);
                         LoggerContext context = (LoggerContext) LogManager.getContext(false);
                         Configurator.shutdown(context);
                     } catch (IOException ex) {
@@ -258,8 +258,8 @@ final class Bootstrap {
     }
 
     static void stop() throws IOException {
-        try (Spawner spawnerToClose = INSTANCE.spawner) {
-            IOUtils.close(INSTANCE.node);
+        try {
+            IOUtils.close(INSTANCE.node, INSTANCE.spawner);
         } finally {
             INSTANCE.keepAliveLatch.countDown();
         }

Also, we are probably missing a logging shutdown in the stop method, but let's worry about that separately.

This comment has been minimized.

Copy link
@droberts195

droberts195 May 10, 2017

Author Contributor

Thanks for the suggestion @jasontedor

@jasontedor
Copy link
Member

left a comment

LGTM.

@droberts195 droberts195 merged commit d611ab4 into elastic:master May 10, 2017

1 of 2 checks passed

elasticsearch-ci Build finished.
Details
CLA Commit author has signed the CLA
Details

@droberts195 droberts195 deleted the droberts195:avoid_controller_shutdown_race branch May 10, 2017

droberts195 added a commit that referenced this pull request May 10, 2017
Avoid race when shutting down controller processes (#24579)
This commit terminates any controller processes plugins might have after
the node has been closed.  This gives the plugins a chance to shut down their
controllers gracefully.

Previously there was a race condition where controller processes could be shut
down gracefully and terminated by two threads running in parallel, leading to
non-deterministic outcomes.

Additionally, controller processes that failed to shut down gracefully were
not forcibly terminated when running as a Windows service; there was a reliance
on the plugin to shut down its controller gracefully in this situation.
This commit also fixes this problem.
droberts195 added a commit that referenced this pull request May 10, 2017
Avoid race when shutting down controller processes (#24579)
This commit terminates any controller processes plugins might have after
the node has been closed.  This gives the plugins a chance to shut down their
controllers gracefully.

Previously there was a race condition where controller processes could be shut
down gracefully and terminated by two threads running in parallel, leading to
non-deterministic outcomes.

Additionally, controller processes that failed to shut down gracefully were
not forcibly terminated when running as a Windows service; there was a reliance
on the plugin to shut down its controller gracefully in this situation.
This commit also fixes this problem.

@clintongormley clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.