Clean up SessionStateImpl lifecycle 46/101946/2
authorRobert Varga <robert.varga@pantheon.tech>
Tue, 2 Aug 2022 19:49:04 +0000 (21:49 +0200)
committerRobert Varga <robert.varga@pantheon.tech>
Tue, 2 Aug 2022 20:09:24 +0000 (22:09 +0200)
SessionStateImpl is inherently bound to statistics updater, hence
it needs to follow bind/unbind lifecycle. This has the benefit of
better isolation -- which is demonstrated by removal of unneeded
listenerState clearing.

On the side of TopologyStatsProviderImpl, we are gaining the benefits of
Registration atomicity, and therefore have a better reign over
invoke-once semantics.

JIRA: BGPCEP-1005
Change-Id: I1b58fc9891c0bfb2a7084cc6eccf9c56b41cd8fc
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractTopologySessionListener.java
pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologySessionListener.java
pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/TopologySessionStatsRegistry.java
pcep/topology/topology-provider/src/main/java/org/opendaylight/bgpcep/pcep/topology/provider/TopologyStatsProviderImpl.java
pcep/topology/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/AbstractPCEPSessionTest.java
pcep/topology/topology-provider/src/test/java/org/opendaylight/bgpcep/pcep/topology/provider/PCEPTopologySessionListenerTest.java

index ebebc3c3515328adde3fc8d1fe5916efc98c7fa0..dd66889b14ea9bea8f9ade79dfe4260733cb9af4 100644 (file)
@@ -7,6 +7,7 @@
  */
 package org.opendaylight.bgpcep.pcep.topology.provider;
 
+import static com.google.common.base.Verify.verifyNotNull;
 import static java.util.Objects.requireNonNull;
 
 import com.google.common.collect.Iterables;
@@ -69,6 +70,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.topology
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.topology.pcep.rev220730.pcep.client.attributes.path.computation.client.reported.lsp.Path;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.topology.pcep.rev220730.pcep.client.attributes.path.computation.client.reported.lsp.PathKey;
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.Node;
+import org.opendaylight.yangtools.concepts.ObjectRegistration;
 import org.opendaylight.yangtools.yang.binding.DataObject;
 import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 import org.opendaylight.yangtools.yang.common.RpcResult;
@@ -91,7 +93,7 @@ public abstract class AbstractTopologySessionListener implements TopologySession
     @GuardedBy("this")
     final Map<PlspId, String> lsps = new HashMap<>();
     @GuardedBy("this")
-    SessionStateImpl listenerState;
+    private ObjectRegistration<SessionStateImpl> listenerState;
 
     // FIXME: clarify lifecycle rules of this map, most notably the interaction of multiple SrpIdNumbers
     @GuardedBy("this")
@@ -153,7 +155,8 @@ public abstract class AbstractTopologySessionListener implements TopologySession
                 session = psession;
                 nodeState = state;
 
-                LOG.trace("Peer {} resolved to topology node {}", peerAddress, state.getNodeId());
+                final var nodeId = state.getNodeId();
+                LOG.trace("Peer {} resolved to topology node {}", peerAddress, nodeId);
 
                 // Our augmentation in the topology node
                 final PathComputationClientBuilder pccBuilder = new PathComputationClientBuilder()
@@ -164,7 +167,7 @@ public abstract class AbstractTopologySessionListener implements TopologySession
 
                 synced.set(isSynchronized());
 
-                final InstanceIdentifier<Node1> topologyAugment = state.getNodeId().augmentation(Node1.class);
+                final InstanceIdentifier<Node1> topologyAugment = nodeId.augmentation(Node1.class);
                 pccIdentifier = topologyAugment.child(PathComputationClient.class);
 
                 if (haveLspDbVersion) {
@@ -178,11 +181,8 @@ public abstract class AbstractTopologySessionListener implements TopologySession
                 state.storeNode(topologyAugment,
                         new Node1Builder().setPathComputationClient(pccBuilder.build()).build(), psession);
 
-                // TODO: collapse assignment? needs to be verified through bytecode
-                final var sessionState = new SessionStateImpl(this, psession);
-                listenerState = sessionState;
-                statsProvider.bind(state.getNodeId(), sessionState);
-                LOG.info("Session with {} attached to topology node {}", peerAddress, state.getNodeId());
+                listenerState = statsProvider.bind(nodeId, new SessionStateImpl(this, psession));
+                LOG.info("Session with {} attached to topology node {}", peerAddress, nodeId);
             }
         }
     }
@@ -280,7 +280,6 @@ public abstract class AbstractTopologySessionListener implements TopologySession
                     LOG.error("Session {} cannot be closed.", psession, e);
                 }
                 session = null;
-                listenerState = null;
                 syncOptimization = null;
                 clearRequests();
             }
@@ -362,7 +361,6 @@ public abstract class AbstractTopologySessionListener implements TopologySession
                     session.close(TerminationReason.UNKNOWN);
                     session = null;
                 }
-                listenerState = null;
                 syncOptimization = null;
                 clearRequests();
             }
@@ -372,8 +370,11 @@ public abstract class AbstractTopologySessionListener implements TopologySession
     @Holding({"this.serverSessionManager", "this"})
     private void clearNodeState() {
         if (nodeState != null) {
-            statsProvider.unbind(nodeState.getNodeId());
             LOG.debug("Clear Node state: {}", nodeState.getNodeId());
+            if (listenerState != null) {
+                listenerState.close();
+                listenerState = null;
+            }
             nodeState = null;
         }
     }
@@ -408,7 +409,8 @@ public abstract class AbstractTopologySessionListener implements TopologySession
     final synchronized PCEPRequest removeRequest(final SrpIdNumber id) {
         final PCEPRequest ret = requests.remove(id);
         if (ret != null && listenerState != null) {
-            listenerState.processRequestStats(ret.getElapsedMillis());
+            // FIXME: just update fields
+            listenerState.getInstance().processRequestStats(ret.getElapsedMillis());
         }
         LOG.trace("Removed request {} object {}", id, ret);
         return ret;
@@ -417,7 +419,8 @@ public abstract class AbstractTopologySessionListener implements TopologySession
     final synchronized ListenableFuture<OperationResult> sendMessage(final Message message, final SrpIdNumber requestId,
             final Metadata metadata) {
         final var sendFuture = session.sendMessage(message);
-        listenerState.updateStatefulSentMsg(message);
+        // FIXME: just update fields
+        listenerState().updateStatefulSentMsg(message);
 
         // Note: the timeout is held back by us holding the 'this' monitor, which timeoutExpired re-acquires
         final var timeout = serverSessionManager.newRpcTimeout(this::timeoutExpired, requestId);
@@ -680,13 +683,16 @@ public abstract class AbstractTopologySessionListener implements TopologySession
         return lspUpdateCapability.get();
     }
 
-
     @Override
     public synchronized ListenableFuture<RpcResult<Void>> tearDownSession(final TearDownSessionInput input) {
         close();
         return RpcResultBuilder.<Void>success().buildFuture();
     }
 
+    final synchronized @NonNull SessionStateImpl listenerState() {
+        return verifyNotNull(listenerState).getInstance();
+    }
+
     static final class MessageContext {
         private final Collection<PCEPRequest> requests = new ArrayList<>();
         private final WriteTransaction trans;
index cf15a3e1d357c69feab01e24e9720bb153ab8228..e2965ccd747980599a22dae416b868151431db97 100644 (file)
@@ -402,7 +402,8 @@ class PCEPTopologySessionListener extends AbstractTopologySessionListener {
         if (!(message instanceof PcrptMessage)) {
             return true;
         }
-        listenerState.updateLastReceivedRptMsg();
+        // FIXME: update just a field
+        listenerState().updateLastReceivedRptMsg();
         final var rpt = ((PcrptMessage) message).getPcrptMessage();
         for (final Reports report : rpt.nonnullReports()) {
             if (!manageNextReport(report, ctx)) {
index 81be0a5a5dbc1b8770261f20f1a075045e4ab998..c6f31143397f8ebd1a881d48e7023e748cffc5b8 100644 (file)
@@ -7,10 +7,12 @@
  */
 package org.opendaylight.bgpcep.pcep.topology.provider;
 
+import edu.umd.cs.findbugs.annotations.CheckReturnValue;
 import org.eclipse.jdt.annotation.NonNull;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.stats.rev171113.PcepSessionState;
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.Node;
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.NodeKey;
+import org.opendaylight.yangtools.concepts.ObjectRegistration;
 import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier;
 
 /**
@@ -23,12 +25,8 @@ interface TopologySessionStatsRegistry {
      * @param nodeId       Identifier of the topology node where it will be stored session stats under DS
      * @param sessionState containing all Stats Session information
      */
-    void bind(@NonNull KeyedInstanceIdentifier<Node, NodeKey> nodeId, @NonNull PcepSessionState sessionState);
-
-    /**
-     * Unregister Node from Stats Registry handler.
-     *
-     * @param nodeId Identifier of the topology node to be removed from registry
-     */
-    void unbind(@NonNull KeyedInstanceIdentifier<Node, NodeKey> nodeId);
+    // FIXME: BGPCEP-1105: nodeId is a bit superfluous, lifecycle is driven by AbstractTopologySessionListener
+    @CheckReturnValue
+    <T extends PcepSessionState> @NonNull ObjectRegistration<T> bind(
+        @NonNull KeyedInstanceIdentifier<Node, NodeKey> nodeId, @NonNull T sessionState);
 }
index 960685439be373eacd5c906e27bd26fe012c8edd..5e2b5ade2f4ec43bc17f7ccf6dfaa720d6015be0 100644 (file)
@@ -14,7 +14,6 @@ import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.MoreExecutors;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Set;
 import java.util.TimerTask;
 import java.util.concurrent.ConcurrentHashMap;
@@ -25,6 +24,7 @@ import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
 import org.checkerframework.checker.lock.qual.GuardedBy;
 import org.checkerframework.checker.lock.qual.Holding;
+import org.eclipse.jdt.annotation.NonNull;
 import org.eclipse.jdt.annotation.Nullable;
 import org.opendaylight.mdsal.binding.api.DataBroker;
 import org.opendaylight.mdsal.binding.api.Transaction;
@@ -39,6 +39,9 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.topology
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.topology.pcep.stats.rev181109.PcepTopologyNodeStatsAugBuilder;
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.Node;
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.NodeKey;
+import org.opendaylight.yangtools.concepts.AbstractObjectRegistration;
+import org.opendaylight.yangtools.concepts.NoOpObjectRegistration;
+import org.opendaylight.yangtools.concepts.ObjectRegistration;
 import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -57,7 +60,7 @@ public final class TopologyStatsProviderImpl implements TopologySessionStatsRegi
     //        retry in face of failing transactions.
     private final Set<KeyedInstanceIdentifier<Node, NodeKey>> statsPendingDelete = ConcurrentHashMap.newKeySet();
     @GuardedBy("this")
-    private final Map<KeyedInstanceIdentifier<Node, NodeKey>, PcepSessionState> statsMap = new HashMap<>();
+    private final Map<KeyedInstanceIdentifier<Node, NodeKey>, Reg<?>> statsMap = new HashMap<>();
     // Note: null indicates we have been shut down
     @GuardedBy("this")
     private DataBroker dataBroker;
@@ -139,13 +142,16 @@ public final class TopologyStatsProviderImpl implements TopologySessionStatsRegi
 
         final WriteTransaction tx = chain.newWriteOnlyTransaction();
         try {
-            for (Entry<KeyedInstanceIdentifier<Node, NodeKey>, PcepSessionState> entry : statsMap.entrySet()) {
+            for (var entry : statsMap.entrySet()) {
                 if (!statsPendingDelete.contains(entry.getKey())) {
-                    tx.put(LogicalDatastoreType.OPERATIONAL,
+                    final var reg = entry.getValue();
+                    if (reg.notClosed()) {
+                        tx.put(LogicalDatastoreType.OPERATIONAL,
                             entry.getKey().augmentation(PcepTopologyNodeStatsAug.class),
                             new PcepTopologyNodeStatsAugBuilder()
-                                    .setPcepSessionState(new PcepSessionStateBuilder(entry.getValue()).build())
-                                    .build());
+                                .setPcepSessionState(new PcepSessionStateBuilder(reg.getInstance()).build())
+                                .build());
+                    }
                 }
             }
         } catch (Exception e) {
@@ -186,17 +192,29 @@ public final class TopologyStatsProviderImpl implements TopologySessionStatsRegi
     }
 
     @Override
-    public synchronized void bind(final KeyedInstanceIdentifier<Node, NodeKey> nodeId,
-            final PcepSessionState sessionState) {
-        if (dataBroker != null) {
-            statsMap.put(nodeId, sessionState);
-        } else {
+    public synchronized <T extends PcepSessionState> ObjectRegistration<T> bind(
+            final KeyedInstanceIdentifier<Node, NodeKey> nodeId, final T sessionState) {
+        if (dataBroker == null) {
             LOG.debug("Ignoring bind of Pcep Node {}", nodeId);
+            return NoOpObjectRegistration.of(sessionState);
         }
+
+        final var ret = new Reg<>(sessionState, nodeId);
+        // FIXME: a replace should never happen, and hence regs are just a Set (which can be concurrent and this method
+        //        does not need synchronization
+        statsMap.put(nodeId, ret);
+        return ret;
     }
 
-    @Override
-    public synchronized void unbind(final KeyedInstanceIdentifier<Node, NodeKey> nodeId) {
+    private synchronized void removeRegistration(final @NonNull Reg<?> reg) {
+        final var nodeId = reg.nodeId;
+
+        if (!statsMap.remove(nodeId, reg)) {
+            // Already replaced by a subsequent bind()
+            LOG.debug("Ignoring overridden unbind of Pcep Node {}", nodeId);
+            return;
+        }
+
         final TransactionChain chain = accessChain();
         if (chain == null) {
             // Already closed, do not bother
@@ -204,12 +222,6 @@ public final class TopologyStatsProviderImpl implements TopologySessionStatsRegi
             return;
         }
 
-        final PcepSessionState node = statsMap.remove(nodeId);
-        if (node == null) {
-            LOG.debug("Ignoring duplicate unbind of Pcep Node {}", nodeId);
-            return;
-        }
-
         statsPendingDelete.add(nodeId);
         final WriteTransaction wTx = chain.newWriteOnlyTransaction();
         wTx.delete(LogicalDatastoreType.OPERATIONAL, nodeId);
@@ -227,4 +239,18 @@ public final class TopologyStatsProviderImpl implements TopologySessionStatsRegi
             }
         }, MoreExecutors.directExecutor());
     }
+
+    private final class Reg<T extends PcepSessionState> extends AbstractObjectRegistration<T> {
+        private final @NonNull KeyedInstanceIdentifier<Node, NodeKey> nodeId;
+
+        Reg(final @NonNull T instance, final KeyedInstanceIdentifier<Node, NodeKey> nodeId) {
+            super(instance);
+            this.nodeId = requireNonNull(nodeId);
+        }
+
+        @Override
+        protected void removeRegistration() {
+            TopologyStatsProviderImpl.this.removeRegistration(this);
+        }
+    }
 }
index 085ae973b2f0db9fe0994935e456fdbf5c3051b4..ca56fc3a7725891a77084d2c240001785cc427b6 100644 (file)
@@ -11,7 +11,6 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doAnswer;
-import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 
@@ -38,6 +37,7 @@ import org.opendaylight.protocol.util.InetSocketAddressUtil;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.inet.types.rev130715.IpPrefix;
 import org.opendaylight.yang.gen.v1.urn.ietf.params.xml.ns.yang.ietf.inet.types.rev130715.Ipv4Prefix;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.graph.rev220720.graph.topology.GraphKey;
+import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.stats.rev171113.PcepSessionState;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.types.rev181109.explicit.route.object.Ero;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.types.rev181109.explicit.route.object.EroBuilder;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.pcep.types.rev181109.explicit.route.object.ero.Subobject;
@@ -57,6 +57,7 @@ import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.TopologyKey;
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.Node;
 import org.opendaylight.yang.gen.v1.urn.tbd.params.xml.ns.yang.network.topology.rev131021.network.topology.topology.NodeKey;
+import org.opendaylight.yangtools.concepts.NoOpObjectRegistration;
 import org.opendaylight.yangtools.yang.binding.InstanceIdentifier;
 import org.opendaylight.yangtools.yang.binding.KeyedInstanceIdentifier;
 import org.opendaylight.yangtools.yang.binding.Notification;
@@ -119,8 +120,8 @@ public abstract class AbstractPCEPSessionTest extends AbstractConcurrentDataBrok
         doReturn(pipeline).when(pipeline).replace(any(ChannelHandler.class), any(String.class),
             any(ChannelHandler.class));
         doReturn(eventLoop).when(clientListener).eventLoop();
-        doNothing().when(statsRegistry).bind(any(), any());
-        doNothing().when(statsRegistry).unbind(any());
+        doAnswer(inv -> NoOpObjectRegistration.of(inv.getArgument(1, PcepSessionState.class)))
+            .when(statsRegistry).bind(any(), any());
         doReturn(null).when(eventLoop).schedule(any(Runnable.class), any(long.class), any(TimeUnit.class));
         doReturn(true).when(clientListener).isActive();
         final InetSocketAddress ra = new InetSocketAddress(testAddress, 4189);
index 200638246b752bd117bc424bb0d0be2e41bfd63d..5cd1d4d3a8f5507c0d8fb4a8eb5b710ad66f6f7c 100644 (file)
@@ -95,6 +95,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.topology
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.topology.pcep.rev220730.pcep.client.attributes.path.computation.client.ReportedLsp;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.topology.pcep.rev220730.pcep.client.attributes.path.computation.client.reported.lsp.Path;
 import org.opendaylight.yangtools.yang.common.RpcResult;
+import org.opendaylight.yangtools.yang.common.Uint16;
 import org.opendaylight.yangtools.yang.common.Uint32;
 
 public class PCEPTopologySessionListenerTest extends AbstractPCEPSessionTest {
@@ -117,20 +118,19 @@ public class PCEPTopologySessionListenerTest extends AbstractPCEPSessionTest {
     @Test
     public void testPCEPTopologySessionListener() throws Exception {
         listener.onSessionUp(session);
-        final PcepSessionState listenerState = listener.listenerState;
-        assertEquals(testAddress, listenerState.getPeerPref().getIpAddress());
-        final LocalPref state = listener.listenerState.getLocalPref();
+        final PcepSessionState listenerState = listener.listenerState();
+        final LocalPref state = listenerState.getLocalPref();
         assertNotNull(state);
         assertEquals(DEAD_TIMER, state.getDeadtimer().shortValue());
         assertEquals(KEEP_ALIVE, state.getKeepalive().shortValue());
-        assertEquals(0, state.getSessionId().intValue());
+        assertEquals(Uint16.ZERO, state.getSessionId());
         assertEquals(testAddress, state.getIpAddress());
 
         final PeerPref peerState = listenerState.getPeerPref();
-
+        assertEquals(testAddress, peerState.getIpAddress());
         assertEquals(DEAD_TIMER, peerState.getDeadtimer().shortValue());
         assertEquals(KEEP_ALIVE, peerState.getKeepalive().shortValue());
-        assertEquals(0, peerState.getSessionId().intValue());
+        assertEquals(Uint16.ZERO, peerState.getSessionId());
         assertEquals(testAddress, peerState.getIpAddress());
 
         // add-lsp
@@ -610,7 +610,7 @@ public class PCEPTopologySessionListenerTest extends AbstractPCEPSessionTest {
                 .build(), Optional.of(MsgBuilderUtil.createSrp(srpId)), MsgBuilderUtil.createPath(
                 req.getEro().getSubobject()));
         listener.onMessage(session, pcRpt);
-        checkEquals(() -> assertEquals(1, listener.listenerState.getDelegatedLspsCount().intValue()));
+        checkEquals(() -> assertEquals(Uint16.ONE, listener.listenerState().getDelegatedLspsCount()));
     }
 
     @Test
@@ -634,7 +634,7 @@ public class PCEPTopologySessionListenerTest extends AbstractPCEPSessionTest {
                         .build(), Optional.of(MsgBuilderUtil.createSrp(srpId)),
                 MsgBuilderUtil.createPath(req.getEro().getSubobject()));
         listener.onMessage(session, pcRpt);
-        checkEquals(() -> assertEquals(0, listener.listenerState.getDelegatedLspsCount().intValue()));
+        checkEquals(() -> assertEquals(Uint16.ZERO, listener.listenerState().getDelegatedLspsCount()));
     }
 
     @Override