From ac9b359c3fce7bbb99392841daf776c33a234f6d Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Wed, 20 Mar 2024 14:03:55 +0100 Subject: [PATCH] Cleanup up registration lifecycle We have a StackOverflowError during shutdown caused by recursive invocation. This stems from a lifecycle misunderstanding. Remove close() methods and use plain yangtools.concepts.Registration to have clear semantics. Change-Id: I356f9f5e7bd179799b7ca5e081714d02d4ee0903 Signed-off-by: Robert Varga --- .../frm/impl/DeviceMastershipManager.java | 3 +- .../impl/ReconciliationManagerImpl.java | 10 ++-- .../MastershipChangeRegistration.java | 1 + .../mastership/MastershipChangeService.java | 4 +- .../MastershipChangeServiceManager.java | 13 ++--- .../ReconciliationFrameworkEvent.java | 6 +- .../ReconciliationFrameworkRegistration.java | 1 + .../MastershipChangeServiceManagerImpl.java | 32 ++++++++--- .../mastership/MastershipServiceDelegate.java | 2 +- ...econciliationFrameworkServiceDelegate.java | 55 ------------------- .../lifecycle/ContextChainHolderImplTest.java | 3 +- ...astershipChangeServiceManagerImplTest.java | 24 ++++---- 12 files changed, 50 insertions(+), 104 deletions(-) delete mode 100644 openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/ReconciliationFrameworkServiceDelegate.java diff --git a/applications/forwardingrules-manager/src/main/java/org/opendaylight/openflowplugin/applications/frm/impl/DeviceMastershipManager.java b/applications/forwardingrules-manager/src/main/java/org/opendaylight/openflowplugin/applications/frm/impl/DeviceMastershipManager.java index 08a1943bad..8356a1d7bf 100644 --- a/applications/forwardingrules-manager/src/main/java/org/opendaylight/openflowplugin/applications/frm/impl/DeviceMastershipManager.java +++ b/applications/forwardingrules-manager/src/main/java/org/opendaylight/openflowplugin/applications/frm/impl/DeviceMastershipManager.java @@ -29,7 +29,6 @@ import org.opendaylight.mdsal.binding.api.DataTreeModification; import org.opendaylight.mdsal.binding.api.RpcProviderService; import org.opendaylight.mdsal.common.api.LogicalDatastoreType; import org.opendaylight.openflowplugin.api.openflow.device.DeviceInfo; -import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeRegistration; import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeService; import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeServiceManager; import org.opendaylight.openflowplugin.applications.frm.FlowNodeReconciliation; @@ -66,7 +65,7 @@ public class DeviceMastershipManager implements DataTreeChangeListener decidedResultState = new AtomicReference<>(ResultState.DONOTHING); @@ -51,7 +51,7 @@ public final class ReconciliationManagerImpl implements ReconciliationManager, R new ConcurrentSkipListMap<>(); private final ConcurrentMap> futureMap = new ConcurrentHashMap<>(); private final ConcurrentMap resultStateMap = new ConcurrentHashMap<>(); - private final ReconciliationFrameworkRegistration reg; + private final Registration reg; @Inject @Activate @@ -64,7 +64,7 @@ public final class ReconciliationManagerImpl implements ReconciliationManager, R @PreDestroy @Deactivate @Override - public void close() throws Exception { + public void close() { reg.close(); LOG.info("ReconciliationManager stopped"); } diff --git a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeRegistration.java b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeRegistration.java index 7eca8fc372..8589faee81 100644 --- a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeRegistration.java +++ b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeRegistration.java @@ -18,5 +18,6 @@ package org.opendaylight.openflowplugin.api.openflow.mastership; * @see MastershipChangeService * @since 0.5.0 Nitrogen */ +@Deprecated(forRemoval = true) public interface MastershipChangeRegistration extends AutoCloseable { } diff --git a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeService.java b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeService.java index c2f248ef67..ef06e687c6 100644 --- a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeService.java +++ b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeService.java @@ -23,8 +23,7 @@ import org.opendaylight.openflowplugin.api.openflow.lifecycle.OwnershipChangeLis * with the device in both cases. * @since 0.5.0 Nitrogen */ -public interface MastershipChangeService extends AutoCloseable { - +public interface MastershipChangeService { /** * Event when device is ready as a master. This event is evoked by * {@link OwnershipChangeListener#becomeMaster(DeviceInfo)} @@ -38,5 +37,4 @@ public interface MastershipChangeService extends AutoCloseable { * @param deviceInfo connected switch identification */ void onLoseOwnership(@NonNull DeviceInfo deviceInfo); - } \ No newline at end of file diff --git a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeServiceManager.java b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeServiceManager.java index 5795e388c9..ec86c6cb19 100644 --- a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeServiceManager.java +++ b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/MastershipChangeServiceManager.java @@ -9,14 +9,14 @@ package org.opendaylight.openflowplugin.api.openflow.mastership; import org.eclipse.jdt.annotation.NonNull; import org.opendaylight.openflowplugin.api.openflow.lifecycle.OwnershipChangeListener; +import org.opendaylight.yangtools.concepts.Registration; /** * Provider to register mastership change listener. * Provider to set mastership reconciliation framework. * @since 0.5.0 Nitrogen */ -public interface MastershipChangeServiceManager extends OwnershipChangeListener, AutoCloseable { - +public interface MastershipChangeServiceManager extends OwnershipChangeListener { /** * Register of mastership change listener. Returned registration need to be closed by client. * It doesn't contain event for reconciliation framework event. @@ -25,7 +25,7 @@ public interface MastershipChangeServiceManager extends OwnershipChangeListener, * @see ReconciliationFrameworkEvent */ @NonNull - MastershipChangeRegistration register(@NonNull MastershipChangeService service); + Registration register(@NonNull MastershipChangeService service); /** * Setter for reconciliation framework event listener. It can be registered only once. @@ -34,9 +34,6 @@ public interface MastershipChangeServiceManager extends OwnershipChangeListener, * @return registration object, which can be closed to unregister * @throws MastershipChangeException if already reconciliation framework registered */ - ReconciliationFrameworkRegistration reconciliationFrameworkRegistration( - @NonNull ReconciliationFrameworkEvent mastershipRFRegistration) throws MastershipChangeException; - - @Override - void close(); + Registration reconciliationFrameworkRegistration(@NonNull ReconciliationFrameworkEvent mastershipRFRegistration) + throws MastershipChangeException; } diff --git a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/ReconciliationFrameworkEvent.java b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/ReconciliationFrameworkEvent.java index 44aa8187ee..38f086398d 100644 --- a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/ReconciliationFrameworkEvent.java +++ b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/ReconciliationFrameworkEvent.java @@ -27,9 +27,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.openflow * and application can't do anything with node before the device is not stored in to data store. * @since 0.5.0 Nitrogen */ - -public interface ReconciliationFrameworkEvent extends AutoCloseable { - +public interface ReconciliationFrameworkEvent { /** * Event when device is ready as a master but not yet submitted in data store. This event is evoked by * {@link OwnershipChangeListener#becomeMasterBeforeSubmittedDS(DeviceInfo)} @@ -47,6 +45,4 @@ public interface ReconciliationFrameworkEvent extends AutoCloseable { * @see MastershipChangeService */ ListenableFuture onDeviceDisconnected(@NonNull DeviceInfo deviceInfo); - - } diff --git a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/ReconciliationFrameworkRegistration.java b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/ReconciliationFrameworkRegistration.java index 0d9ff6a637..edf2ba094c 100644 --- a/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/ReconciliationFrameworkRegistration.java +++ b/openflowplugin-api/src/main/java/org/opendaylight/openflowplugin/api/openflow/mastership/ReconciliationFrameworkRegistration.java @@ -17,5 +17,6 @@ package org.opendaylight.openflowplugin.api.openflow.mastership; * @see ReconciliationFrameworkEvent * @since 0.5.0 Nitrogen */ +@Deprecated(forRemoval = true) public interface ReconciliationFrameworkRegistration extends AutoCloseable { } diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/MastershipChangeServiceManagerImpl.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/MastershipChangeServiceManagerImpl.java index 64c6497c87..06ecc97a30 100644 --- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/MastershipChangeServiceManagerImpl.java +++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/MastershipChangeServiceManagerImpl.java @@ -7,6 +7,7 @@ */ package org.opendaylight.openflowplugin.impl.mastership; +import static java.util.Objects.requireNonNull; import static org.opendaylight.infrautils.utils.concurrent.LoggingFutures.addErrorLogging; import com.google.common.annotations.VisibleForTesting; @@ -21,12 +22,12 @@ import org.eclipse.jdt.annotation.NonNull; import org.opendaylight.openflowplugin.api.openflow.device.DeviceInfo; import org.opendaylight.openflowplugin.api.openflow.lifecycle.MasterChecker; import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeException; -import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeRegistration; import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeService; import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeServiceManager; import org.opendaylight.openflowplugin.api.openflow.mastership.ReconciliationFrameworkEvent; -import org.opendaylight.openflowplugin.api.openflow.mastership.ReconciliationFrameworkRegistration; import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.openflowplugin.rf.state.rev170713.ResultState; +import org.opendaylight.yangtools.concepts.AbstractRegistration; +import org.opendaylight.yangtools.concepts.Registration; import org.osgi.service.component.annotations.Activate; import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Deactivate; @@ -35,7 +36,7 @@ import org.slf4j.LoggerFactory; @Singleton @Component(immediate = true) -public final class MastershipChangeServiceManagerImpl implements MastershipChangeServiceManager { +public final class MastershipChangeServiceManagerImpl implements MastershipChangeServiceManager, AutoCloseable { private static final Logger LOG = LoggerFactory.getLogger(MastershipChangeServiceManagerImpl.class); private final List serviceGroup = new CopyOnWriteArrayList<>(); @@ -57,9 +58,15 @@ public final class MastershipChangeServiceManagerImpl implements MastershipChang } @Override - public MastershipChangeRegistration register(final MastershipChangeService service) { - final var registration = new MastershipServiceDelegate(service, () -> serviceGroup.remove(service)); - serviceGroup.add(service); + public Registration register(final MastershipChangeService service) { + serviceGroup.add(requireNonNull(service)); + final var registration = new AbstractRegistration() { + @Override + protected void removeRegistration() { + serviceGroup.remove(service); + } + }; + if (masterChecker != null && masterChecker.isAnyDeviceMastered()) { masterChecker.listOfMasteredDevices().forEach(service::onBecomeOwner); } @@ -67,13 +74,20 @@ public final class MastershipChangeServiceManagerImpl implements MastershipChang } @Override - public ReconciliationFrameworkRegistration reconciliationFrameworkRegistration( + public synchronized Registration reconciliationFrameworkRegistration( final ReconciliationFrameworkEvent reconciliationFrameworkEvent) throws MastershipChangeException { if (rfService != null) { throw new MastershipChangeException("Reconciliation framework already registered."); } - rfService = reconciliationFrameworkEvent; - return new ReconciliationFrameworkServiceDelegate(reconciliationFrameworkEvent, () -> rfService = null); + rfService = requireNonNull(reconciliationFrameworkEvent); + return new AbstractRegistration() { + @Override + protected void removeRegistration() { + synchronized (MastershipChangeServiceManagerImpl.this) { + rfService = null; + } + } + }; } @Override diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/MastershipServiceDelegate.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/MastershipServiceDelegate.java index 25e9fb728d..5fbd4742df 100644 --- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/MastershipServiceDelegate.java +++ b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/MastershipServiceDelegate.java @@ -14,8 +14,8 @@ import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeS import org.slf4j.Logger; import org.slf4j.LoggerFactory; +@Deprecated public class MastershipServiceDelegate implements MastershipChangeService, MastershipChangeRegistration { - private static final Logger LOG = LoggerFactory.getLogger(MastershipServiceDelegate.class); private final MastershipChangeService service; diff --git a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/ReconciliationFrameworkServiceDelegate.java b/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/ReconciliationFrameworkServiceDelegate.java deleted file mode 100644 index af3c7a8342..0000000000 --- a/openflowplugin-impl/src/main/java/org/opendaylight/openflowplugin/impl/mastership/ReconciliationFrameworkServiceDelegate.java +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright (c) 2017 Pantheon Technologies s.r.o. and others. All rights reserved. - * - * This program and the accompanying materials are made available under the - * terms of the Eclipse Public License v1.0 which accompanies this distribution, - * and is available at http://www.eclipse.org/legal/epl-v10.html - */ -package org.opendaylight.openflowplugin.impl.mastership; - -import com.google.common.util.concurrent.ListenableFuture; -import org.eclipse.jdt.annotation.NonNull; -import org.opendaylight.openflowplugin.api.openflow.device.DeviceInfo; -import org.opendaylight.openflowplugin.api.openflow.mastership.ReconciliationFrameworkEvent; -import org.opendaylight.openflowplugin.api.openflow.mastership.ReconciliationFrameworkRegistration; -import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.openflowplugin.rf.state.rev170713.ResultState; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -public class ReconciliationFrameworkServiceDelegate implements - ReconciliationFrameworkEvent, ReconciliationFrameworkRegistration { - - private static final Logger LOG = LoggerFactory.getLogger(ReconciliationFrameworkServiceDelegate.class); - - private final ReconciliationFrameworkEvent service; - private final AutoCloseable unregister; - - ReconciliationFrameworkServiceDelegate(final ReconciliationFrameworkEvent service, - final AutoCloseable unregisterService) { - LOG.debug("Reconciliation framework service registered: {}", service); - this.service = service; - this.unregister = unregisterService; - } - - @Override - public void close() throws Exception { - LOG.debug("Reconciliation framework service un-registered: {}", service); - this.unregister.close(); - this.service.close(); - } - - @Override - public ListenableFuture onDevicePrepared(@NonNull DeviceInfo deviceInfo) { - return this.service.onDevicePrepared(deviceInfo); - } - - @Override - public ListenableFuture onDeviceDisconnected(@NonNull DeviceInfo deviceInfo) { - return this.service.onDeviceDisconnected(deviceInfo); - } - - @Override - public String toString() { - return service.toString(); - } -} diff --git a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/ContextChainHolderImplTest.java b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/ContextChainHolderImplTest.java index a4b80f653e..85bba48177 100644 --- a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/ContextChainHolderImplTest.java +++ b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/lifecycle/ContextChainHolderImplTest.java @@ -28,7 +28,6 @@ import org.opendaylight.openflowplugin.api.openflow.device.DeviceManager; import org.opendaylight.openflowplugin.api.openflow.lifecycle.ContextChainMastershipState; import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeServiceManager; import org.opendaylight.openflowplugin.api.openflow.mastership.ReconciliationFrameworkEvent; -import org.opendaylight.openflowplugin.api.openflow.mastership.ReconciliationFrameworkRegistration; import org.opendaylight.openflowplugin.api.openflow.role.RoleContext; import org.opendaylight.openflowplugin.api.openflow.role.RoleManager; import org.opendaylight.openflowplugin.api.openflow.rpc.RpcContext; @@ -89,7 +88,7 @@ public class ContextChainHolderImplTest { private OpenflowProviderConfig config; private ContextChainHolderImpl contextChainHolder; - private ReconciliationFrameworkRegistration registration; + private Registration registration; private final MastershipChangeServiceManager manager = new MastershipChangeServiceManagerImpl(); @Before diff --git a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/mastership/MastershipChangeServiceManagerImplTest.java b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/mastership/MastershipChangeServiceManagerImplTest.java index 69f3c185c9..48e558c957 100644 --- a/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/mastership/MastershipChangeServiceManagerImplTest.java +++ b/openflowplugin-impl/src/test/java/org/opendaylight/openflowplugin/impl/mastership/MastershipChangeServiceManagerImplTest.java @@ -20,11 +20,10 @@ import org.mockito.junit.MockitoJUnitRunner; import org.opendaylight.openflowplugin.api.openflow.device.DeviceInfo; import org.opendaylight.openflowplugin.api.openflow.lifecycle.MasterChecker; import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeException; -import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeRegistration; import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeService; import org.opendaylight.openflowplugin.api.openflow.mastership.MastershipChangeServiceManager; import org.opendaylight.openflowplugin.api.openflow.mastership.ReconciliationFrameworkEvent; -import org.opendaylight.openflowplugin.api.openflow.mastership.ReconciliationFrameworkRegistration; +import org.opendaylight.yangtools.concepts.Registration; @RunWith(MockitoJUnitRunner.class) public class MastershipChangeServiceManagerImplTest { @@ -43,8 +42,8 @@ public class MastershipChangeServiceManagerImplTest { private ReconciliationFrameworkEvent secondEvent; private final MastershipChangeServiceManager manager = new MastershipChangeServiceManagerImpl(); - private MastershipChangeRegistration registration; - private ReconciliationFrameworkRegistration registrationRF; + private Registration registration; + private Registration registrationRF; @Before public void setUp() throws Exception { @@ -61,20 +60,18 @@ public class MastershipChangeServiceManagerImplTest { @Test public void registerTwice() { - MastershipChangeRegistration registration2; - registration2 = manager.register(secondService); + Registration registration2 = manager.register(secondService); Assert.assertNotNull(registration); Assert.assertNotNull(registration2); } @Test public void uregisterTwice() throws Exception { - MastershipChangeRegistration registration2; - registration2 = manager.register(secondService); - Assert.assertTrue(((MastershipChangeServiceManagerImpl)manager).serviceGroupListSize() == 2); - registration.close(); - Assert.assertTrue(((MastershipChangeServiceManagerImpl)manager).serviceGroupListSize() == 1); - registration2.close(); + try (var registration2 = manager.register(secondService)) { + Assert.assertTrue(((MastershipChangeServiceManagerImpl)manager).serviceGroupListSize() == 2); + registration.close(); + Assert.assertTrue(((MastershipChangeServiceManagerImpl)manager).serviceGroupListSize() == 1); + } Assert.assertTrue(((MastershipChangeServiceManagerImpl)manager).serviceGroupListSize() == 0); } @@ -91,8 +88,7 @@ public class MastershipChangeServiceManagerImplTest { @Test public void unregosteringRF() throws Exception { registrationRF.close(); - ReconciliationFrameworkRegistration registration1; - registration1 = manager.reconciliationFrameworkRegistration(secondEvent); + Registration registration1 = manager.reconciliationFrameworkRegistration(secondEvent); Assert.assertNotNull(registration1); } -- 2.36.6