-
Notifications
You must be signed in to change notification settings - Fork 961
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
Avoid deadlock on processing events. #1853
Avoid deadlock on processing events. #1853
Conversation
…itoredNode::OnReportEvent. Refactor MasterNodeManager::GetManagerHandle to avoid calling other node manager's GetManagerHandle inside a lock statement
Codecov Report
@@ Coverage Diff @@
## master #1853 +/- ##
==========================================
- Coverage 55.57% 55.50% -0.08%
==========================================
Files 319 319
Lines 58839 58827 -12
==========================================
- Hits 32701 32650 -51
- Misses 26138 26177 +39
Continue to review full report at Codecov.
|
handle = m_nodeManagers[1].GetManagerHandle(nodeId); | ||
nodeManagers = m_namespaceManagers[index]; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the lock should remain around all loops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it would generally safeguard, but this is a deadlock waiting to happen (and actually happened in #1851) since CustomNodeManager2.GetManagerHandle locks its public Lock, which besides being used also in all derived node managers, its also used by MonitoredNode and ServerInternalData.
Since m_nodeManagers is modified during startup and no other thread apparently operates on the List, I think it is safe to unwrap it from the Lock(m_namespaceManagers.SyncRoot) as the code stands now.
I see no other timely feasible workaround from this deadlock state (#1851).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could use a clone (shallow copy) of the nodeManagers = m_namespaceManagers[index].Clone() as INodeManager[]; on the other hand and this would skip the chance of having a race condition over the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better use ReaderWriterLockSlim instead of Lock(m_namespaceManagers.SyncRoot) , there are rare cases when the nodemanager array changes, so most of the time threads can start to get the manager handle concurrently.
This pull request introduces 1 alert when merging 6009d53 into 93e549a - view on LGTM.com new alerts:
|
…LockSlim instance
commit 73435df Author: Randy Armstrong <[email protected]> Date: Sat Oct 15 23:07:34 2022 +0900 Updates for 1.05.02. commit 746195c Merge: 6d950c7 da1b1a7 Author: Randy Armstrong <[email protected]> Date: Fri Oct 14 22:29:31 2022 +0900 Resync with baseline. commit da1b1a7 Author: Martin Regen <[email protected]> Date: Tue Oct 11 11:03:00 2022 +0200 Add Session.CloseAsync, fix UANodesetHelper.Save (#1944) -Add Session.CloseAsync methods. -Add missing Close signatures to ISession -Mark the CloseSession service call as obsolete to prevent invalid usage -fix UANodeSetHelper.Save(stream), to not close the stream -fix a deadlock in the ref server app, due to the new default behavior of the cert validator not caching the approved certificates commit 1f2a30d Author: Ivan van Heerden <[email protected]> Date: Fri Oct 7 20:11:53 2022 +0200 Add abstraction 'ISession' for Session object in the Client library (#1702) Expose an 'ISession' interface as abstraction to ease unit testing. Breaking changes are kept to minimal impact, but some changes maybe required to use ISession objects instead of Session. Note: ISessionClientMethods is hand created, not yet by ModelCompiler commit 72842f1 Author: Martin Regen <[email protected]> Date: Thu Oct 6 12:14:39 2022 +0200 New self signed certificates set CA flag to false (#1941) Based on discussions in the security WG the default for the CA flag of self signed certificates is now 'false'. - see https://mantis.opcfoundation.org/view.php?id=8370 commit 77b8950 Author: Martin Regen <[email protected]> Date: Thu Oct 6 10:53:32 2022 +0200 Use of validated certificates needs opt-in. (#1925) Currently, the validated certificates are stored in a dictionary for fast compare of the certificate validation when an application reconnects, until manually cleared by e.g. `ResetValidatedCertificates()`. After discussion it was decided to make this function an opt-in feature to avoid use cases where changes in the trusted store or CRL are not reflected in the list of validated certificates. The runtime penalty of a full check can be avoided if needed with the 'Opt-In' by configuration or by application setting the property. After opt-in it is the application responsibility to clear the list of validated certificates if needed. +++ Other cert validator improvements/fixes - Setting the `ValidationOptions.SuppressRevocationStatusUnknown` on a trustlist (e.g. issuer) was ignored - If another client/server connects with a chain, the whole chain is saved to the rejected store, not only the leaf certificate. commit 1fec89b Author: Martin Regen <[email protected]> Date: Tue Oct 4 19:45:26 2022 +0200 Complex types improvements (#1915) Improvements for complex types. - Support to encode/decode complex type structures with multidimensional arrays. - Perf improvements for type loader using new NodeCache fetch multiple interfaces. - DataTypeDictionary validation when dependencies are used. - interface for complex type resolver, to build custom types in code. - improvements of console client to demonstrate browse/read of custom values +++ Breaking changes +++ IDecoder interface change of the `ReadArray` method - Return type is now `Array` for all types of arrays. Previously the method returned an array for 1-dim and a `Matrix` type for multidimensional arrays. In addition the convention in generated code is to use `Collection` for all 1-dim arrays, and so was built the `BaseComplexType`. The new convention is to expose the native `Array` type from a complex type, in all cases, hence the return type of `ReadArray` was changed to reflect that for a consistent programming model. - `ReadArray` has a new parameter `systemType` to efficiently read encodeable types. Previously the `Type` of an encodeable was determined by the encodingid and lookup in the type factory, even when the Type is already known by the caller. The 'old' style is still supported. - `ReadArray` supports to read encodeable type arrays which are directly encoded in structures. These arrays differ from standard encoding of arrays and they are handled when `BuiltInType.Null` is used as parameter. commit 6d950c7 Author: Randy Armstrong <[email protected]> Date: Sat Oct 1 23:12:14 2022 +0900 1.05.02 preparations. commit 8be757f Author: Randy Armstrong <[email protected]> Date: Sat Oct 1 21:37:59 2022 +0900 Add support for Roles. commit 82e94f6 Merge: 3eed300 677edb9 Author: Randy Armstrong <[email protected]> Date: Sat Oct 1 21:36:26 2022 +0900 Merge remote-tracking branch 'source/master' commit 677edb9 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue Sep 27 07:12:04 2022 +0200 Bump Microsoft.NET.Test.Sdk from 17.3.1 to 17.3.2 (#1937) Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 17.3.1 to 17.3.2. - [Release notes](https://github.com/microsoft/vstest/releases) - [Commits](microsoft/vstest@v17.3.1...v17.3.2) --- updated-dependencies: - dependency-name: Microsoft.NET.Test.Sdk dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 95c384f Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon Sep 26 10:41:18 2022 +0300 Bump Microsoft.Extensions.Logging.Abstractions from 6.0.1 to 6.0.2 (#1932) * Bump Microsoft.Extensions.Logging.Abstractions from 6.0.1 to 6.0.2 Bumps [Microsoft.Extensions.Logging.Abstractions](https://github.com/dotnet/runtime) from 6.0.1 to 6.0.2. - [Release notes](https://github.com/dotnet/runtime/releases) - [Commits](dotnet/runtime@v6.0.1...v6.0.2) --- updated-dependencies: - dependency-name: Microsoft.Extensions.Logging.Abstractions dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * Update Opc.Ua.Core.csproj * Update UA Client Controls.csproj * Update UA Server Controls.csproj Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Martin Regen <[email protected]> commit b42d377 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed Sep 14 17:24:19 2022 +0300 Bump Serilog from 2.11.0 to 2.12.0 (#1931) Bumps [Serilog](https://github.com/serilog/serilog) from 2.11.0 to 2.12.0. - [Release notes](https://github.com/serilog/serilog/releases) - [Changelog](https://github.com/serilog/serilog/blob/dev/CHANGES.md) - [Commits](serilog/serilog@v2.11.0...v2.12.0) --- updated-dependencies: - dependency-name: Serilog dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 441fbae Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed Sep 14 14:53:00 2022 +0300 Bump NunitXml.TestLogger from 3.0.107 to 3.0.127 (#1929) Bumps [NunitXml.TestLogger](https://github.com/spekt/nunit.testlogger) from 3.0.107 to 3.0.127. - [Release notes](https://github.com/spekt/nunit.testlogger/releases) - [Changelog](https://github.com/spekt/nunit.testlogger/blob/master/CHANGELOG.md) - [Commits](spekt/nunit.testlogger@v3.0.107...v3.0.127) --- updated-dependencies: - dependency-name: NunitXml.TestLogger dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 0e4f8f3 Merge: 29b1bbe 35adf40 Author: Suciu Mircea Adrian <[email protected]> Date: Wed Sep 7 16:59:22 2022 +0300 Merge pull request #1926 from OPCFoundation/dependabot/nuget/Serilog.Sinks.Console-4.1.0 Bump Serilog.Sinks.Console from 4.0.1 to 4.1.0 commit 35adf40 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue Sep 6 10:01:43 2022 +0000 Bump Serilog.Sinks.Console from 4.0.1 to 4.1.0 Bumps [Serilog.Sinks.Console](https://github.com/serilog/serilog-sinks-console) from 4.0.1 to 4.1.0. - [Release notes](https://github.com/serilog/serilog-sinks-console/releases) - [Commits](serilog/serilog-sinks-console@v4.0.1...v4.1.0) --- updated-dependencies: - dependency-name: Serilog.Sinks.Console dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> commit 29b1bbe Author: TeaTime762 <[email protected]> Date: Wed Aug 31 08:41:36 2022 +0200 Rethrow exception on MasterNodeManager StartUp (#1906) * So far errors during startup of MasterNodeManager were consumed and only logged, so the app did not know there is an issue * New behavior is to rethrow exception to retrieve it outside the NuGet package * Update MasterNodeManager.cs commit 885c7b2 Author: Martin Regen <[email protected]> Date: Tue Aug 30 22:18:15 2022 +0200 Fix server url mismatch handling (#1914) * change the Certificate domain validation to work also for the server case * do not refuse a connection if the check fails, only trigger the audit event * bump up test Nuget commit 0f5ad5e Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Aug 26 20:13:18 2022 +0200 Bump BenchmarkDotNet and CI VM versions (#1922) * bump up VM versions * Bump BenchmarkDotNet from 0.13.1 to 0.13.2 Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Martin Regen <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit d0fb11d Author: Martin Regen <[email protected]> Date: Mon Aug 22 13:52:21 2022 +0200 Set higher default client chunk count limit (#1916) -Issue: A server might return a much smaller send and receive buffer size than what was proposed in the hello message by a client. Given the current default max chunk count, which is based on the default buffer size, the response message size is also limited by the calculated max chunk count. - fix: Calculate the default max chunk counts based on the TCP client min buffer size. commit f273634 Author: Martin Regen <[email protected]> Date: Sat Aug 20 23:44:09 2022 +0200 Improve https transport and other bug fixes (#1908) -Make the https callbacks fully async / code cleanup. -Default https client to use MaxConnectionsToServer = 64 for #1550 -Fix https missing reconnect error code other fixes unrelated to https: - move trust list audit event to AuditEvents.cs, fix issue that the event is not filtered since reportAuditEvent cannot be used - fix an issue when BrowseNext for multiple continuation tokens doesn't add the results array properly - bump up .NET Core 3.1 Nuget dependencies - improve .NET reference client app for use with https commit 346e7a6 Author: opcfoundation-org <[email protected]> Date: Thu Aug 18 07:49:24 2022 -0700 JSON objects cannot have the DataTypeEncodingId (#1911) *The EncodableFactory does not index the JSON DataTypeEncodingIds because they are not defined on the types. As a result, a JSON document that complies with the specification (see https://reference.opcfoundation.org/Core/Part6/5.4.2/#5.4.2.16) cannot be parsed. * This change creates the JSON encoding types ids from generated code using reflection, so the JSON documents which comply with the spec can be decoded. * Fixes type id on a JSON encoded ExtensionObject that contains binary or xml encoded data. Co-authored-by: Randy Armstrong <[email protected]> commit 0a9a3f3 Author: Suciu Mircea Adrian <[email protected]> Date: Mon Aug 15 14:04:22 2022 +0300 Fix a few audit events for CTT pass (#1910) - Fixed ReportAuditCreateSessionEvent - removed unnecessary calls to ReportAuditAddNodesEvent, ReportAuditDeleteNodesEvent - added ReportAuditUpdateMethodEvent commit 885a61b Author: Cristian Kohlmann <[email protected]> Date: Fri Aug 12 15:22:28 2022 -0300 Use HashSet for validation cache in MasterNodeManager (#1907) To improve Contains method performance I changed List type for HashSet. commit 0ad5c4c Author: Martin Regen <[email protected]> Date: Fri Aug 12 20:04:02 2022 +0200 Refactor audit events (#1895) - Move the auditing flag to server internal - Hook up new configuration setting to the Auditing node - A Security Admin can change the Audit setting. - Refactor audit events to be extensions in one class. - enable auditing and diagnostics by default for higher test coverage. - refactor dependency of some events to bindings, e.g. TcpServerChannel should not be used as reportevent parameter commit c492950 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu Aug 11 11:08:39 2022 +0200 Bump Microsoft.NET.Test.Sdk from 17.2.0 to 17.3.0 (#1902) Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 17.2.0 to 17.3.0. - [Release notes](https://github.com/microsoft/vstest/releases) - [Commits](microsoft/vstest@v17.2.0...v17.3.0) --- updated-dependencies: - dependency-name: Microsoft.NET.Test.Sdk dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 8147552 Author: OctaviaStan <[email protected]> Date: Wed Aug 10 13:45:38 2022 +0300 - Refactor the ReceiveEvents permission type validation to be taken into account also when ConditionRefresh method is called. (#1899) - Fix error in validating PermissionType.ReceiveEvents. The specs state that "A Client only receives an Event if this bit is set on the Node identified by the EventTypeId field and on the Node identified by the SourceNode field. This Permission is only valid for EventType Nodes or SourceNodes." The previous code was only checking the Node id of event state instance. commit dd78c0b Author: Martin Regen <[email protected]> Date: Mon Aug 8 11:43:55 2022 +0200 Client perf updates for NodeCache and service call layer to batch Operation Limits. (#1876) - Add methods to support to Read/Browse multiple nodes in session and NodeCache. - Add methods to find references of multiple nodes in NodeCache. - By default the client fetches the operation limits from the server. - Add operation limit object to client config, settings are overwritten by server limits if server limits are smaller. - Split client service calls if operation limits are exceeded with an override of the SessionClient. All sync/async service calls are implemented, but not the servcie calls using the Begin/End pattern. commit 5a77423 Author: Martin Regen <[email protected]> Date: Mon Aug 8 05:38:24 2022 +0200 Update codeql action script to v2 (#1894) * bump up abstraction logging 3.1.27, add references to solve some warnings commit 9b05714 Author: Suciu Mircea Adrian <[email protected]> Date: Sun Aug 7 18:44:59 2022 +0300 Handle processing matrixes with array dimensions that overflow. (#1891) commit 931b9b9 Author: OctaviaStan <[email protected]> Date: Sun Aug 7 18:43:43 2022 +0300 Audit events; add methods for reporting Add/Delete nodes audit events & fixes (#1890) * Audit events; add methods for reporting Add/Delete nodes audit events and call these methods in CreateNode and DeleteNode methods - fix values for AuditActivateSessionEventState.ClientSoftwareCertificates and AuditWriteUpdateEventState.IndexRange properties that were set to values of wrong type and generated exception when the client used filters for these properties. commit d4da420 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu Aug 4 10:52:39 2022 +0200 Bump Moq from 4.18.1 to 4.18.2 (#1884) Bumps [Moq](https://github.com/moq/moq4) from 4.18.1 to 4.18.2. - [Release notes](https://github.com/moq/moq4/releases) - [Changelog](https://github.com/moq/moq4/blob/main/CHANGELOG.md) - [Commits](devlooped/moq@v4.18.1...v4.18.2) --- updated-dependencies: - dependency-name: Moq dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 313aa2a Author: Suciu Mircea Adrian <[email protected]> Date: Tue Aug 2 10:36:48 2022 +0300 Allow diagnostic nodes to be accessed by determined users (#1878) commit 1d123da Author: Suciu Mircea Adrian <[email protected]> Date: Tue Jul 19 11:20:04 2022 +0300 Bump to version to 1.4.370 (#1871) commit 5732286 Author: Tim Jöhnk <[email protected]> Date: Mon Jul 18 11:50:36 2022 +0200 Feature/recursive complex types (#1870) * fixes recursive complex types * fix IComplexTypebuilder interface commit 8c0111b Author: Joshua Leger <[email protected]> Date: Sun Jul 17 12:21:07 2022 -0400 Adding UnregisterNamespaceManager support to MasterNodeManager (#1869) * Introducing MasterNodeManager::UnregisterNamespaceManager function commit 274dfad Author: Suciu Mircea Adrian <[email protected]> Date: Sun Jul 17 18:41:43 2022 +0300 Implementation of audit event triggers (#1867) * Implementation of audit event triggers commit cf0c878 Author: Vincenzo Carlino <[email protected]> Date: Sun Jul 17 06:19:11 2022 +0200 Add methods for reading multiple nodes (#1522) * Add new methods for reading a set of nodes * ReadNodes(NodeIdCollection, out DataValueCollection, out IList<ServiceResult>, bool optionalAttributes) * (DataValueCollection, IList<ServiceResult>) ReadNodesAsync (NodeIdCollection, bool optionalAttributes) to reduce the amount of attributes read from a server ReadNodes first reads only the NodeClass, then reads only the mandatory attributes per nodeclass to reduce the overall number of attributes to read. If the NodeClass of all nodes is already known, the read can be further optimized to read only the attributes for a known class: * ReadNodes(NodeIdCollection, NodeClass, out DataValueCollection, out IList<ServiceResult>, bool optionalAttributes) * (DataValueCollection, IList<ServiceResult>) ReadNodesAsync (NodeIdCollection, NodeClass, bool optionalAttributes) to complete async and collection operation, there is a: * ReadNodeAsync and * ReadValues/ReadValuesAsync * An issue was fixed in the https async codepath, the security header was not properly set in async operation sidenote: operation limits must be handled by caller! Co-authored-by: Martin Regen <[email protected]> commit 8fc68e3 Author: Martin Regen <[email protected]> Date: Fri Jul 1 08:19:57 2022 +0200 return value also for uncertain (#1861) commit 559c382 Author: Zigby <[email protected]> Date: Thu Jun 30 14:02:07 2022 +0200 Fix Server JWT Token Support (#1817) * Handle JWT and set the IssuedTokenType in the server based on the policy referenced by the PolicyId in the client supplied UserIdentityToken . * Connect/Reconnect tests cases with JWT commit e07cba1 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu Jun 30 13:51:07 2022 +0200 Bump NUnit.Console from 3.15.0 to 3.15.2 (#1860) Bumps [NUnit.Console](https://github.com/nunit/nunit-console) from 3.15.0 to 3.15.2. - [Release notes](https://github.com/nunit/nunit-console/releases) - [Changelog](https://github.com/nunit/nunit-console/blob/main/CHANGES.txt) - [Commits](nunit/nunit-console@3.15.0...3.15.2) --- updated-dependencies: - dependency-name: NUnit.Console dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 10d4dbc Author: Martin Regen <[email protected]> Date: Wed Jun 29 06:41:59 2022 +0200 Improve ResendData server support (#1854) * add resend data support to sample node manager * After WG discussion, ensure after ResendData only a single value per monitored item is sent in the next publish (sync client cache) * Seperate resend data state from ready to publish with IsResendData property commit c0fc029 Author: Suciu Mircea Adrian <[email protected]> Date: Tue Jun 21 11:53:47 2022 +0300 Avoid deadlock on processing events. (#1853) * Moved validation of RolePermission from MonitoredItem::Publish to MonitoredNode::OnReportEvent. Refactor MasterNodeManager::GetManagerHandle to avoid calling other node manager's GetManagerHandle inside a lock statement commit df71776 Author: opcfoundation-org <[email protected]> Date: Tue Jun 21 01:51:41 2022 -0700 Modelcompiler 1.04.11 (#1850) * latest nodeset without reference to servicemodel
Proposed changes
Moved validation of RolePermission from MonitoredItem::Publish to MonitoredNode::OnReportEvent.
Refactor MasterNodeManager::GetManagerHandle to avoid calling other node manager's GetManagerHandle inside a lock statement.
Related Issues
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.