Skip to content

Commit

Permalink
Make NodeState classes thread safe (#2372)
Browse files Browse the repository at this point in the history 
NodeState classes are not thread safe. In many places event handlers are handled in a manner that can cause NullReferenceException in a case of a bad timing. In addition, in some places non-thread-safe collections are iterated which could cause issues if the collections are changed at the same time.

This PR addresses the aforementioned issues by using local variables for event handlers and then making null check and calling them. When applicable null-conditional operators are used. Non-thread-safe collection iteration is guarded by locking. The issues were reproduces with unit tests part of this PR and also the fix was verified by running the same unit tests.
  • Loading branch information
@saurla
saurla authored Feb 29, 2024
1 parent 76fe9c9 commit 3f29a72
Show file tree
Hide file tree
Showing 12 changed files with 1,427 additions and 400 deletions.
38 changes: 23 additions & 15 deletions Stack/Opc.Ua.Core/Stack/State/BaseInstanceState.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,9 @@ public string GetDisplayPath(int maxLength, char seperator)
{
string name = GetNonNullText(this);

if (m_parent == null)
NodeState stateParent = m_parent;

if (stateParent == null)
{
return name;
}
Expand All @@ -130,7 +132,7 @@ public string GetDisplayPath(int maxLength, char seperator)

if (maxLength > 2)
{
NodeState parent = m_parent;
NodeState parent = stateParent;
List<string> names = new List<string>();

while (parent != null)
Expand Down Expand Up @@ -158,7 +160,7 @@ public string GetDisplayPath(int maxLength, char seperator)
}
}

buffer.Append(GetNonNullText(m_parent));
buffer.Append(GetNonNullText(stateParent));
buffer.Append(seperator);
buffer.Append(name);

Expand Down Expand Up @@ -272,10 +274,8 @@ public override void ReportEvent(ISystemContext context, IFilterTarget e)
base.ReportEvent(context, e);

// recusively notify the parent.
if (m_parent != null)
{
m_parent.ReportEvent(context, e);
}
m_parent?.ReportEvent(context, e);

}

/// <summary>
Expand Down Expand Up @@ -691,29 +691,37 @@ protected override void PopulateBrowser(ISystemContext context, NodeBrowser brow
{
base.PopulateBrowser(context, browser);

if (!NodeId.IsNull(m_typeDefinitionId) && IsObjectOrVariable)
NodeId typeDefinitionId = m_typeDefinitionId;

if (!NodeId.IsNull(typeDefinitionId) && IsObjectOrVariable)
{
if (browser.IsRequired(ReferenceTypeIds.HasTypeDefinition, false))
{
browser.Add(ReferenceTypeIds.HasTypeDefinition, false, m_typeDefinitionId);
browser.Add(ReferenceTypeIds.HasTypeDefinition, false, typeDefinitionId);
}
}

if (!NodeId.IsNull(m_modellingRuleId))
NodeId modellingRuleId = m_modellingRuleId;

if (!NodeId.IsNull(modellingRuleId))
{
if (browser.IsRequired(ReferenceTypeIds.HasModellingRule, false))
{
browser.Add(ReferenceTypeIds.HasModellingRule, false, m_modellingRuleId);
browser.Add(ReferenceTypeIds.HasModellingRule, false, modellingRuleId);
}
}

if (m_parent != null)
NodeState parent = m_parent;

if (parent != null)
{
if (!NodeId.IsNull(m_referenceTypeId))
NodeId referenceTypeId = this.m_referenceTypeId;

if (!NodeId.IsNull(referenceTypeId))
{
if (browser.IsRequired(m_referenceTypeId, true))
if (browser.IsRequired(referenceTypeId, true))
{
browser.Add(m_referenceTypeId, true, m_parent);
browser.Add(referenceTypeId, true, parent);
}
}
}
Expand Down
12 changes: 8 additions & 4 deletions Stack/Opc.Ua.Core/Stack/State/BaseObjectState.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,9 +269,11 @@ protected override ServiceResult ReadNonValueAttribute(
{
byte eventNotifier = m_eventNotifier;

if (OnReadEventNotifier != null)
NodeAttributeEventHandler<byte> readEventNotifier = OnReadEventNotifier;

if (readEventNotifier != null)
{
result = OnReadEventNotifier(context, this, ref eventNotifier);
result = readEventNotifier(context, this, ref eventNotifier);
}

if (ServiceResult.IsGood(result))
Expand Down Expand Up @@ -316,9 +318,11 @@ protected override ServiceResult WriteNonValueAttribute(

byte eventNotifier = eventNotifierRef.Value;

if (OnWriteEventNotifier != null)
NodeAttributeEventHandler<byte> writeEventNotifier = OnWriteEventNotifier;

if (writeEventNotifier != null)
{
result = OnWriteEventNotifier(context, this, ref eventNotifier);
result = writeEventNotifier(context, this, ref eventNotifier);
}

if (ServiceResult.IsGood(result))
Expand Down
24 changes: 16 additions & 8 deletions Stack/Opc.Ua.Core/Stack/State/BaseTypeState.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,9 +302,11 @@ protected override ServiceResult ReadNonValueAttribute(
{
bool isAbstract = m_isAbstract;

if (OnReadIsAbstract != null)
NodeAttributeEventHandler<bool> onReadIsAbstract = OnReadIsAbstract;

if (onReadIsAbstract != null)
{
result = OnReadIsAbstract(context, this, ref isAbstract);
result = onReadIsAbstract(context, this, ref isAbstract);
}

if (ServiceResult.IsGood(result))
Expand Down Expand Up @@ -349,9 +351,11 @@ protected override ServiceResult WriteNonValueAttribute(

bool isAbstract = isAbstractRef.Value;

if (OnWriteIsAbstract != null)
NodeAttributeEventHandler<bool> onWriteIsAbstract = OnWriteIsAbstract;

if (onWriteIsAbstract != null)
{
result = OnWriteIsAbstract(context, this, ref isAbstract);
result = onWriteIsAbstract(context, this, ref isAbstract);
}

if (ServiceResult.IsGood(result))
Expand All @@ -377,20 +381,24 @@ protected override void PopulateBrowser(ISystemContext context, NodeBrowser brow
{
base.PopulateBrowser(context, browser);

if (!NodeId.IsNull(m_superTypeId))
NodeId superTypeId = m_superTypeId;

if (!NodeId.IsNull(superTypeId))
{
if (browser.IsRequired(ReferenceTypeIds.HasSubtype, true))
{
browser.Add(ReferenceTypeIds.HasSubtype, true, m_superTypeId);
browser.Add(ReferenceTypeIds.HasSubtype, true, superTypeId);
}
}

NodeId nodeId = this.NodeId;

// use the type table to find the subtypes.
if (context.TypeTable != null && this.NodeId != null)
if (context.TypeTable != null && nodeId != null)
{
if (browser.IsRequired(ReferenceTypeIds.HasSubtype, false))
{
IList<NodeId> subtypeIds = context.TypeTable.FindSubTypes(this.NodeId);
IList<NodeId> subtypeIds = context.TypeTable.FindSubTypes(nodeId);

for (int ii = 0; ii < subtypeIds.Count; ii++)
{
Expand Down
Loading

0 comments on commit 3f29a72

Please sign in to comment.