From dc661bac4655d335bae2857eba89a13e7cafecdf Mon Sep 17 00:00:00 2001 From: Martin Regen <7962757+mregen@users.noreply.github.com> Date: Fri, 13 Dec 2024 08:59:28 +0100 Subject: [PATCH] Fix server CurrentTime source timestamp and improve readerwriter locks (#2903) Fix server time source timestamp is not updated. There are duplicate objects in generated code, on of which which holds the last timestamp was not updated. Move all readerwriter locks outside of try/finally to avoid a SynchronizationLockException when a reader writer lock is disposed or is cancelled. Currently there is no obvious fundamental bug in how the readerwriter locks are used, but outside the try/finally clause e.g. a disposed lock may not trigger a false exception. --- .../Opc.Ua.Client/NodeCache/NodeCache.cs | 94 ++++++++----------- .../Opc.Ua.Client/NodeCache/NodeCacheAsync.cs | 27 +++--- .../NodeManager/MasterNodeManager.cs | 9 +- .../Server/ServerInternalData.cs | 7 ++ .../Types/Encoders/EncodeableFactory.cs | 14 ++- 5 files changed, 68 insertions(+), 83 deletions(-) diff --git a/Libraries/Opc.Ua.Client/NodeCache/NodeCache.cs b/Libraries/Opc.Ua.Client/NodeCache/NodeCache.cs index 00dddacd2e..d71d362414 100644 --- a/Libraries/Opc.Ua.Client/NodeCache/NodeCache.cs +++ b/Libraries/Opc.Ua.Client/NodeCache/NodeCache.cs @@ -104,10 +104,10 @@ public INode Find(ExpandedNodeId nodeId) } INode node; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - // check if node already exists. node = m_nodes.Find(nodeId); } @@ -155,10 +155,10 @@ public IList Find(IList nodeIds) for (ii = 0; ii < count; ii++) { INode node; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - // check if node already exists. node = m_nodes.Find(nodeIds[ii]); } @@ -235,10 +235,10 @@ public INode Find( } IList references; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - // find all references. references = source.ReferenceTable.Find(referenceTypeId, isInverse, includeSubtypes, m_typeTree); } @@ -285,10 +285,9 @@ public IList Find( } IList references; + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - // find all references. references = source.ReferenceTable.Find(referenceTypeId, isInverse, includeSubtypes, m_typeTree); } @@ -325,10 +324,9 @@ public bool IsKnown(ExpandedNodeId typeId) return false; } + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - return m_typeTree.IsKnown(typeId); } finally @@ -347,10 +345,9 @@ public bool IsKnown(NodeId typeId) return false; } + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - return m_typeTree.IsKnown(typeId); } finally @@ -369,10 +366,9 @@ public NodeId FindSuperType(ExpandedNodeId typeId) return null; } + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - return m_typeTree.FindSuperType(typeId); } finally @@ -392,10 +388,9 @@ public NodeId FindSuperType(NodeId typeId) return null; } + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - return m_typeTree.FindSuperType(typeId); } finally @@ -409,18 +404,18 @@ public NodeId FindSuperType(NodeId typeId) public IList FindSubTypes(ExpandedNodeId typeId) { ILocalNode type = Find(typeId) as ILocalNode; + List subtypes = new List(); if (type == null) { - return new List(); + return subtypes; } - List subtypes = new List(); IList references; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - references = type.References.Find(ReferenceTypeIds.HasSubtype, false, true, m_typeTree); } finally @@ -459,10 +454,10 @@ public bool IsTypeOf(ExpandedNodeId subTypeId, ExpandedNodeId superTypeId) while (supertype != null) { ExpandedNodeId currentId; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - currentId = supertype.References.FindTarget(ReferenceTypeIds.HasSubtype, true, true, m_typeTree, 0); } finally @@ -501,10 +496,10 @@ public bool IsTypeOf(NodeId subTypeId, NodeId superTypeId) while (supertype != null) { ExpandedNodeId currentId; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - currentId = supertype.References.FindTarget(ReferenceTypeIds.HasSubtype, true, true, m_typeTree, 0); } finally @@ -526,10 +521,9 @@ public bool IsTypeOf(NodeId subTypeId, NodeId superTypeId) /// public QualifiedName FindReferenceTypeName(NodeId referenceTypeId) { + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - return m_typeTree.FindReferenceTypeName(referenceTypeId); } finally @@ -541,10 +535,9 @@ public QualifiedName FindReferenceTypeName(NodeId referenceTypeId) /// public NodeId FindReferenceType(QualifiedName browseName) { + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - return m_typeTree.FindReferenceType(browseName); } finally @@ -564,10 +557,10 @@ public bool IsEncodingOf(ExpandedNodeId encodingId, ExpandedNodeId datatypeId) } IList references; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - references = encoding.References.Find(ReferenceTypeIds.HasEncoding, true, true, m_typeTree); } finally @@ -611,10 +604,10 @@ public bool IsEncodingFor(NodeId expectedTypeId, ExtensionObject value) } IList references; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - references = encoding.References.Find(ReferenceTypeIds.HasEncoding, true, true, m_typeTree); } finally @@ -706,10 +699,10 @@ public NodeId FindDataTypeId(ExpandedNodeId encodingId) } IList references; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - references = encoding.References.Find(ReferenceTypeIds.HasEncoding, true, true, m_typeTree); } finally @@ -736,10 +729,10 @@ public NodeId FindDataTypeId(NodeId encodingId) } IList references; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - references = encoding.References.Find(ReferenceTypeIds.HasEncoding, true, true, m_typeTree); } finally @@ -769,10 +762,9 @@ public void LoadUaDefinedTypes(ISystemContext context) var assembly = typeof(ArgumentCollection).GetTypeInfo().Assembly; predefinedNodes.LoadFromBinaryResource(context, "Opc.Ua.Stack.Generated.Opc.Ua.PredefinedNodes.uanodes", assembly, true); + m_cacheLock.EnterWriteLock(); try { - m_cacheLock.EnterWriteLock(); - for (int ii = 0; ii < predefinedNodes.Count; ii++) { BaseTypeState type = predefinedNodes[ii] as BaseTypeState; @@ -795,11 +787,10 @@ public void LoadUaDefinedTypes(ISystemContext context) /// public void Clear() { - m_uaTypesLoaded = false; + m_cacheLock.EnterWriteLock(); try { - m_cacheLock.EnterWriteLock(); - + m_uaTypesLoaded = false; m_nodes.Clear(); } finally @@ -826,10 +817,9 @@ public Node FetchNode(ExpandedNodeId nodeId) // fetch references from server. ReferenceDescriptionCollection references = m_session.FetchReferences(localId); + m_cacheLock.EnterUpgradeableReadLock(); try { - m_cacheLock.EnterUpgradeableReadLock(); - foreach (ReferenceDescription reference in references) { // create a placeholder for the node if it does not already exist. @@ -896,10 +886,9 @@ public IList FetchNodes(IList nodeIds) foreach (ReferenceDescription reference in references) { + m_cacheLock.EnterUpgradeableReadLock(); try { - m_cacheLock.EnterUpgradeableReadLock(); - // create a placeholder for the node if it does not already exist. if (!m_nodes.Exists(reference.NodeId)) { @@ -976,10 +965,10 @@ public IList FindReferences( } IList references; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - references = source.ReferenceTable.Find(referenceTypeId, isInverse, includeSubtypes, m_typeTree); } finally @@ -1026,10 +1015,10 @@ public IList FindReferences( foreach (var referenceTypeId in referenceTypeIds) { IList references; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - references = node.ReferenceTable.Find(referenceTypeId, isInverse, includeSubtypes, m_typeTree); } finally @@ -1077,10 +1066,10 @@ public string GetDisplayText(INode node) NodeId modellingRule = target.ModellingRule; IList references; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - references = target.ReferenceTable.Find(ReferenceTypeIds.Aggregates, true, true, m_typeTree); } finally @@ -1167,10 +1156,9 @@ public NodeId BuildBrowsePath(ILocalNode node, IList browsePath) #region Private Methods private void InternalWriteLockedAttach(ILocalNode node) { + m_cacheLock.EnterWriteLock(); try { - m_cacheLock.EnterWriteLock(); - // add to cache. m_nodes.Attach(node); } diff --git a/Libraries/Opc.Ua.Client/NodeCache/NodeCacheAsync.cs b/Libraries/Opc.Ua.Client/NodeCache/NodeCacheAsync.cs index ebd8b1ea2c..2255a85847 100644 --- a/Libraries/Opc.Ua.Client/NodeCache/NodeCacheAsync.cs +++ b/Libraries/Opc.Ua.Client/NodeCache/NodeCacheAsync.cs @@ -52,11 +52,10 @@ public async Task FindAsync(ExpandedNodeId nodeId, CancellationToken ct = return null; } + m_cacheLock.EnterReadLock(); INode node; try { - m_cacheLock.EnterReadLock(); - // check if node already exists. node = m_nodes.Find(nodeId); } @@ -104,10 +103,10 @@ public async Task> FindAsync(IList nodeIds, Cancell for (ii = 0; ii < count; ii++) { INode node; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - // check if node already exists. node = m_nodes.Find(nodeIds[ii]); } @@ -179,10 +178,9 @@ public async Task FindSuperTypeAsync(ExpandedNodeId typeId, Cancellation return null; } + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - return m_typeTree.FindSuperType(typeId); } finally @@ -201,10 +199,9 @@ public async Task FindSuperTypeAsync(NodeId typeId, CancellationToken ct return null; } + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - return m_typeTree.FindSuperType(typeId); } finally @@ -233,10 +230,9 @@ public async Task FetchNodeAsync(ExpandedNodeId nodeId, CancellationToken // fetch references from server. ReferenceDescriptionCollection references = await m_session.FetchReferencesAsync(localId, ct).ConfigureAwait(false); + m_cacheLock.EnterUpgradeableReadLock(); try { - m_cacheLock.EnterUpgradeableReadLock(); - foreach (ReferenceDescription reference in references) { // create a placeholder for the node if it does not already exist. @@ -304,10 +300,9 @@ public async Task> FetchNodesAsync(IList nodeIds, Ca foreach (ReferenceDescription reference in references) { + m_cacheLock.EnterUpgradeableReadLock(); try { - m_cacheLock.EnterUpgradeableReadLock(); - // create a placeholder for the node if it does not already exist. if (!m_nodes.Exists(reference.NodeId)) { @@ -356,10 +351,10 @@ public async Task> FindReferencesAsync( } IList references; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - references = source.ReferenceTable.Find(referenceTypeId, isInverse, includeSubtypes, m_typeTree); } finally @@ -407,10 +402,10 @@ public async Task> FindReferencesAsync( foreach (var referenceTypeId in referenceTypeIds) { IList references; + + m_cacheLock.EnterReadLock(); try { - m_cacheLock.EnterReadLock(); - references = node.ReferenceTable.Find(referenceTypeId, isInverse, includeSubtypes, m_typeTree); } finally diff --git a/Libraries/Opc.Ua.Server/NodeManager/MasterNodeManager.cs b/Libraries/Opc.Ua.Server/NodeManager/MasterNodeManager.cs index 6792c19c8f..f47efe07d8 100644 --- a/Libraries/Opc.Ua.Server/NodeManager/MasterNodeManager.cs +++ b/Libraries/Opc.Ua.Server/NodeManager/MasterNodeManager.cs @@ -421,10 +421,9 @@ public void RegisterNamespaceManager(string namespaceUri, INodeManager nodeManag // allocate a new table (using arrays instead of collections because lookup efficiency is critical). INodeManager[][] namespaceManagers = new INodeManager[m_server.NamespaceUris.Count][]; + m_readWriterLockSlim.EnterWriteLock(); try { - m_readWriterLockSlim.EnterWriteLock(); - // copy existing values. for (int ii = 0; ii < m_namespaceManagers.Length; ii++) { @@ -490,10 +489,9 @@ public bool UnregisterNamespaceManager(string namespaceUri, INodeManager nodeMan // allocate a new table (using arrays instead of collections because lookup efficiency is critical). INodeManager[][] namespaceManagers = new INodeManager[m_server.NamespaceUris.Count][]; + m_readWriterLockSlim.EnterWriteLock(); try { - m_readWriterLockSlim.EnterWriteLock(); - // copy existing values. for (int ii = 0; ii < m_namespaceManagers.Length; ii++) { @@ -559,10 +557,9 @@ public virtual object GetManagerHandle(NodeId nodeId, out INodeManager nodeManag // use the namespace index to select the node manager. int index = nodeId.NamespaceIndex; + m_readWriterLockSlim.EnterReadLock(); try { - m_readWriterLockSlim.EnterReadLock(); - // check if node managers are registered - use the core node manager if unknown. if (index >= m_namespaceManagers.Length || m_namespaceManagers[index] == null) { diff --git a/Libraries/Opc.Ua.Server/Server/ServerInternalData.cs b/Libraries/Opc.Ua.Server/Server/ServerInternalData.cs index ba9f82df27..5bdf33b7e3 100644 --- a/Libraries/Opc.Ua.Server/Server/ServerInternalData.cs +++ b/Libraries/Opc.Ua.Server/Server/ServerInternalData.cs @@ -743,6 +743,13 @@ private void OnReadServerStatus( DateTime now = DateTime.UtcNow; m_serverStatus.Timestamp = now; m_serverStatus.Value.CurrentTime = now; + // update other timestamps in NodeState objects which are used to derive the source timestamp + if (variable is ServerStatusValue serverStatusValue && + serverStatusValue.Variable is ServerStatusState serverStatusState) + { + serverStatusState.Timestamp = now; + serverStatusState.CurrentTime.Timestamp = now; + } } } diff --git a/Stack/Opc.Ua.Core/Types/Encoders/EncodeableFactory.cs b/Stack/Opc.Ua.Core/Types/Encoders/EncodeableFactory.cs index c47c8742f4..bc938ba9a9 100644 --- a/Stack/Opc.Ua.Core/Types/Encoders/EncodeableFactory.cs +++ b/Stack/Opc.Ua.Core/Types/Encoders/EncodeableFactory.cs @@ -339,9 +339,9 @@ public int InstanceId /// The underlying system type to add to the factory public void AddEncodeableType(Type systemType) { + m_readerWriterLockSlim.EnterWriteLock(); try { - m_readerWriterLockSlim.EnterWriteLock(); AddEncodeableType(systemType, null); } finally @@ -365,9 +365,9 @@ public void AddEncodeableType(ExpandedNodeId encodingId, Type systemType) Utils.LogWarning("WARNING: Adding type '{0}' to shared Factory #{1}.", systemType.Name, m_instanceId); } #endif + m_readerWriterLockSlim.EnterWriteLock(); try { - m_readerWriterLockSlim.EnterWriteLock(); m_encodeableTypes[encodingId] = systemType; } finally @@ -402,10 +402,9 @@ public void AddEncodeableTypes(Assembly assembly) } #endif + m_readerWriterLockSlim.EnterWriteLock(); try { - m_readerWriterLockSlim.EnterWriteLock(); - Type[] systemTypes = assembly.GetExportedTypes(); var unboundTypeIds = new Dictionary(); @@ -470,9 +469,9 @@ public void AddEncodeableTypes(Assembly assembly) /// The underlying system types to add to the factory public void AddEncodeableTypes(IEnumerable systemTypes) { + m_readerWriterLockSlim.EnterWriteLock(); try { - m_readerWriterLockSlim.EnterWriteLock(); foreach (var type in systemTypes) { if (type.GetTypeInfo().IsAbstract) @@ -498,10 +497,9 @@ public void AddEncodeableTypes(IEnumerable systemTypes) /// The type id to return the system-type of public Type GetSystemType(ExpandedNodeId typeId) { + m_readerWriterLockSlim.EnterReadLock(); try { - m_readerWriterLockSlim.EnterReadLock(); - Type systemType = null; if (NodeId.IsNull(typeId) || !m_encodeableTypes.TryGetValue(typeId, out systemType)) @@ -535,9 +533,9 @@ public object Clone() { EncodeableFactory clone = new EncodeableFactory(null); + m_readerWriterLockSlim.EnterReadLock(); try { - m_readerWriterLockSlim.EnterReadLock(); foreach (KeyValuePair current in m_encodeableTypes) { clone.m_encodeableTypes.Add(current.Key, current.Value);