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.

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

Jump to bottom

fix SmartLifecycle#stop #360

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix SmartLifecycle#stop #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion ...a/src/main/java/com/alipay/sofa/registry/server/data/bootstrap/DataServerInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public boolean isAutoStartup() {
@Override
public void stop(Runnable runnable) {
runnable.run();
this.isRunning = false;
stop();
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect implementation of SmartLifecycle#stop callback ordering

The current implementation executes the callback before completing the stop procedure, which violates Spring's SmartLifecycle contract. The callback should only be executed after the bean has fully stopped to ensure proper shutdown sequencing of dependent beans.

Apply this fix to ensure correct shutdown order:

  @Override
  public void stop(Runnable runnable) {
-   runnable.run();
    stop();
+   runnable.run();
  }

Additionally, consider enhancing the stop() method to properly cleanup resources:

  @Override
  public void stop() {
+   try {
+     // Add cleanup logic here if needed
+     dataServerBootstrap.stop();
+   } finally {
      this.isRunning = false;
+   }
  }

Committable suggestion was skipped due to low confidence.

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: SmartLifecycle contract violation in stop(Runnable) implementation

The current implementation violates the SmartLifecycle contract by executing the callback before stopping the component. This can lead to race conditions and break Spring's shutdown coordination mechanism.

According to Spring's SmartLifecycle contract:

  1. The component should first perform its shutdown logic
  2. Then execute the callback to signal completion to dependent beans
  3. The callback execution allows Spring to coordinate shutdown across multiple SmartLifecycle beans based on their phases

Here's the correct implementation:

  @Override
  public void stop(Runnable callback) {
-   callback.run();
-   stop();
+   stop();
+   callback.run();
  }

This ensures:

  • Component is properly stopped before signaling completion
  • Dependent beans can safely proceed with their shutdown
  • Spring's shutdown coordination mechanism works as intended
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
stop();
stop();
callback.run();

}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...va/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerInitializerConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,6 @@ public int getPhase() {
@Override
public void stop(Runnable callback) {
callback.run();
this.running.set(false);
stop();
}
Comment on lines +76 to 77
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix the order of operations in stop(Runnable).

The current implementation violates Spring's SmartLifecycle contract. According to the contract, the stop operation should be performed before executing the callback. This ensures that the component is fully stopped before notifying the container.

Apply this diff to fix the order:

  @Override
  public void stop(Runnable callback) {
-   callback.run();
    stop();
+   callback.run();
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
stop();
}
stop();
callback.run();
}

🛠️ Refactor suggestion

Add error handling and logging for the stop operation.

The stop operation lacks error handling and logging. If either the stop() call or callback execution fails, it could leave the system in an inconsistent state.

Consider implementing with proper error handling and logging:

  @Override
  public void stop(Runnable callback) {
+   LOGGER.info("Stopping MetaServer...");
+   try {
      stop();
+     LOGGER.info("MetaServer stopped successfully");
      callback.run();
+   } catch (Exception ex) {
+     LOGGER.error("Error during MetaServer shutdown", ex);
+     throw ex;
+   }
  }

Committable suggestion was skipped due to low confidence.

}
2 changes: 1 addition & 1 deletion ...main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,6 @@ public int getPhase() {
@Override
public void stop(Runnable callback) {
callback.run();
this.running.set(false);
stop();
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Incorrect order of operations in stop(Runnable callback)

The current implementation executes the callback before stopping the server, which violates the Spring Framework's SmartLifecycle contract. The callback should only be executed after the shutdown is complete to properly signal completion to the container.

Apply this diff to fix the order:

  @Override
  public void stop(Runnable callback) {
-   callback.run();
    stop();
+   callback.run();
  }

This ensures that:

  1. The server is fully stopped first
  2. The callback is executed only after shutdown is complete
  3. The container is properly notified of the completion
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
stop();
stop();
callback.run();

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix the order of operations in stop(Runnable).

The current implementation executes the callback before stopping the server, which violates the SmartLifecycle contract. The callback should be invoked after the shutdown is complete to properly signal completion to Spring's lifecycle management.

Apply this diff to fix the order:

  @Override
  public void stop(Runnable callback) {
-   callback.run();
    stop();
+   callback.run();
  }

This ensures that:

  1. The server is properly stopped first
  2. The callback is executed after shutdown is complete
  3. Spring's lifecycle management can properly track the shutdown status
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
stop();
stop();
callback.run();

}
}