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

LifecycleManager should apply PreDestroy actions in the reverse order that PostCreate actions were applied #316

Open
byoffe opened this issue Jul 25, 2016 · 3 comments

Comments

@byoffe
Copy link

byoffe commented Jul 25, 2016

  • Two objects are bound within the same scope (say Singleton).
  • A depends upon B.
  • Both A and B have PostCreate/PreDestroy actions.

The expected behavior is that on startup, B is created (and PostCreated) followed by A being created (and PostCreated).
On shutdown, A should be PreDestroyed before B is PreDestroyed. If the B is destroyed before A, A will fail (as B's resources have been released as part of its PreDestroy)

@brharrington
Copy link
Contributor

What version are you using? See #228

@byoffe
Copy link
Author

byoffe commented Sep 12, 2016

Using governator-core-1.14.0 (com.netflix.governator:governator:1.14.0). I found issue #228 previously, but the implementation has changed materially since that time. See the attached test.

GovernatorStartupTest.zip

@twz123
Copy link

twz123 commented Oct 25, 2016

I also noticed that the PreDestroy actions for singletons are somewhat out of order. I analyzed a bit, and found out that there are different code paths for singletons and eager singletons which use different Deques to store the actions. That's why eager singletons are always destroyed after their singleton counterparts.

Since there is a hard distinction between both, I guess there's a reason for that? Maybe someone could share some insights?

Intuitively, I'd just handle eager singletons the same way as singletons. I tried it out using the below patch, and all tess pass, including the test case provided by @byoffe.

diff --git a/governator-core/src/main/java/com/netflix/governator/internal/PreDestroyMonitor.java b/governator-core/src/main/java/com/netflix/governator/internal/PreDestroyMonitor.java
index e28d6c5..e4cb525 100644
--- a/governator-core/src/main/java/com/netflix/governator/internal/PreDestroyMonitor.java
+++ b/governator-core/src/main/java/com/netflix/governator/internal/PreDestroyMonitor.java
@@ -211,7 +211,8 @@ public class PreDestroyMonitor implements AutoCloseable {
          */
         @Override
         public Boolean visitEagerSingleton() {
-            cleanupActions.addFirst(new ManagedInstanceAction(injectee, lifecycleActions)); 
+            final ScopeCleanupMarker marker = scopeCleaner.singletonMarker;
+            marker.getCleanupAction().add(Providers.of(marker), new ManagedInstanceAction(injectee, lifecycleActions));
             return true;
         }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants