Skip to content

Commit

Permalink
improve server contention, fix a deadlock in binary channel on cleanup
Browse files Browse the repository at this point in the history
  • Loading branch information
@mregen
mregen committed Aug 16, 2024
1 parent 4b822fc commit d588ece
Show file tree
Hide file tree
Showing 5 changed files with 177 additions and 132 deletions.
155 changes: 66 additions & 89 deletions Libraries/Opc.Ua.Server/Session/SessionManager.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
using System.Security.Cryptography.X509Certificates;
using System.Globalization;
using System.Threading.Tasks;
using System.Collections.Concurrent;

namespace Opc.Ua.Server
{
Expand Down Expand Up @@ -62,7 +63,7 @@ public SessionManager(
m_maxHistoryContinuationPoints = configuration.ServerConfiguration.MaxHistoryContinuationPoints;
m_minNonceLength = configuration.SecurityConfiguration.NonceLength;

m_sessions = new Dictionary<NodeId, Session>();
m_sessions = new ConcurrentDictionary<NodeId, Session>(Environment.ProcessorCount, m_maxSessionCount);
m_lastSessionId = BitConverter.ToInt64(Utils.Nonce.CreateNonce(sizeof(long)), 0);

// create a event to signal shutdown.
Expand All @@ -87,17 +88,13 @@ protected virtual void Dispose(bool disposing)
{
if (disposing)
{
List<Session> sessions = null;

lock (m_lock)
{
sessions = new List<Session>(m_sessions.Values);
m_sessions.Clear();
}
// create snapshot of all sessions
var sessions = m_sessions.ToArray();
m_sessions.Clear();

foreach (Session session in sessions)
foreach (var sessionKeyValue in sessions)
{
Utils.SilentDispose(session);
Utils.SilentDispose(sessionKeyValue.Value);

Check warning on line 97 in Libraries/Opc.Ua.Server/Session/SessionManager.cs

View check run for this annotation

Codecov / codecov/patch

Libraries/Opc.Ua.Server/Session/SessionManager.cs#L97 

Added line #L97 was not covered by tests
}

m_shutdownEvent.Set();
Expand Down Expand Up @@ -127,18 +124,16 @@ public virtual void Startup()
/// </summary>
public virtual void Shutdown()
{
lock (m_lock)
{
// stop the monitoring thread.
m_shutdownEvent.Set();
// stop the monitoring thread.
m_shutdownEvent.Set();

// dispose of session objects.
foreach (Session session in m_sessions.Values)
{
session.Dispose();
}
// dispose of session objects using a snapshot.
var sessions = m_sessions.ToArray();
m_sessions.Clear();

m_sessions.Clear();
foreach (var sessionKeyValue in sessions)
{
Utils.SilentDispose(sessionKeyValue.Value);
}
}

Expand Down Expand Up @@ -176,9 +171,11 @@ public virtual Session CreateSession(
// check for same Nonce in another session
if (clientNonce != null)
{
foreach (Session sessionIterator in m_sessions.Values)
// iterate over key/value pairs in the dictionary with a thread safe iterator
foreach (var sessionKeyValueIterator in m_sessions)
{
if (Utils.CompareNonce(sessionIterator.ClientNonce, clientNonce))
byte[] sessionClientNonce = sessionKeyValueIterator.Value?.ClientNonce;
if (Utils.CompareNonce(sessionClientNonce, clientNonce))
{
throw new ServiceResultException(StatusCodes.BadNonceInvalid);
}
Expand All @@ -191,7 +188,7 @@ public virtual Session CreateSession(
{
if (context.ChannelContext.EndpointDescription.SecurityMode != MessageSecurityMode.None)
{
authenticationToken = Utils.IncrementIdentifier(ref m_lastSessionId);
authenticationToken = new NodeId(Utils.IncrementIdentifier(ref m_lastSessionId));
}
}

Expand Down Expand Up @@ -243,7 +240,10 @@ public virtual Session CreateSession(
sessionId = session.Id;

// save session.
m_sessions.Add(authenticationToken, session);
if (!m_sessions.TryAdd(authenticationToken, session))
{
throw new ServiceResultException(StatusCodes.BadTooManySessions);

Check warning on line 245 in Libraries/Opc.Ua.Server/Session/SessionManager.cs

View check run for this annotation

Codecov / codecov/patch

Libraries/Opc.Ua.Server/Session/SessionManager.cs#L245 

Added line #L245 was not covered by tests
}
}

// raise session related event.
Expand Down Expand Up @@ -272,6 +272,12 @@ public virtual bool ActivateSession(
UserIdentityToken newIdentity = null;
UserTokenPolicy userTokenPolicy = null;

// fast path no lock
if (!m_sessions.TryGetValue(authenticationToken, out _))
{
throw new ServiceResultException(StatusCodes.BadSessionIdInvalid);

Check warning on line 278 in Libraries/Opc.Ua.Server/Session/SessionManager.cs

View check run for this annotation

Codecov / codecov/patch

Libraries/Opc.Ua.Server/Session/SessionManager.cs#L278 

Added line #L278 was not covered by tests
}

lock (m_lock)
{
// find session.
Expand Down Expand Up @@ -306,7 +312,6 @@ public virtual bool ActivateSession(
out newIdentity,
out userTokenPolicy);
}

IUserIdentity identity = null;
IUserIdentity effectiveIdentity = null;
ServiceResult error = null;
Expand Down Expand Up @@ -393,23 +398,8 @@ public virtual bool ActivateSession(
/// </remarks>
public virtual void CloseSession(NodeId sessionId)
{
// find the session.
Session session = null;

lock (m_lock)
{
foreach (KeyValuePair<NodeId, Session> current in m_sessions)
{
if (current.Value.Id == sessionId)
{
session = current.Value;
m_sessions.Remove(current.Key);
break;
}
}
}

if (session != null)
// find the session and try to remove it.
if (m_sessions.TryRemove(sessionId, out Session session) && session != null)
{
// raise session related event.
RaiseSessionEvent(session, SessionEventReason.Closing);
Expand All @@ -423,7 +413,6 @@ public virtual void CloseSession(NodeId sessionId)
m_server.ServerDiagnostics.CurrentSessionCount--;
}
}

}

/// <summary>
Expand All @@ -442,44 +431,41 @@ public virtual OperationContext ValidateRequest(RequestHeader requestHeader, Req

try
{
lock (m_lock)
// check for create session request.
if (requestType == RequestType.CreateSession || requestType == RequestType.ActivateSession)
{
// check for create session request.
if (requestType == RequestType.CreateSession || requestType == RequestType.ActivateSession)
{
return new OperationContext(requestHeader, requestType);
}
return new OperationContext(requestHeader, requestType);
}

// find session.
if (!m_sessions.TryGetValue(requestHeader.AuthenticationToken, out session))
// find session.
if (!m_sessions.TryGetValue(requestHeader.AuthenticationToken, out session))
{
EventHandler<ValidateSessionLessRequestEventArgs> handler = m_validateSessionLessRequest;

if (handler != null)
{
EventHandler<ValidateSessionLessRequestEventArgs> handler = m_validateSessionLessRequest;
var args = new ValidateSessionLessRequestEventArgs(requestHeader.AuthenticationToken, requestType);
handler(this, args);

Check warning on line 448 in Libraries/Opc.Ua.Server/Session/SessionManager.cs

View check run for this annotation

Codecov / codecov/patch

Libraries/Opc.Ua.Server/Session/SessionManager.cs#L447-L448 

Added lines #L447 - L448 were not covered by tests

if (handler != null)
if (ServiceResult.IsBad(args.Error))
{
var args = new ValidateSessionLessRequestEventArgs(requestHeader.AuthenticationToken, requestType);
handler(this, args);

if (ServiceResult.IsBad(args.Error))
{
throw new ServiceResultException(args.Error);
}

return new OperationContext(requestHeader, requestType, args.Identity);
throw new ServiceResultException(args.Error);

Check warning on line 452 in Libraries/Opc.Ua.Server/Session/SessionManager.cs

View check run for this annotation

Codecov / codecov/patch

Libraries/Opc.Ua.Server/Session/SessionManager.cs#L452 

Added line #L452 was not covered by tests
}

throw new ServiceResultException(StatusCodes.BadSessionIdInvalid);
return new OperationContext(requestHeader, requestType, args.Identity);

Check warning on line 455 in Libraries/Opc.Ua.Server/Session/SessionManager.cs

View check run for this annotation

Codecov / codecov/patch

Libraries/Opc.Ua.Server/Session/SessionManager.cs#L455 

Added line #L455 was not covered by tests
}

// validate request header.
session.ValidateRequest(requestHeader, requestType);
throw new ServiceResultException(StatusCodes.BadSessionIdInvalid);
}

// validate user has permissions for additional info
session.ValidateDiagnosticInfo(requestHeader);
// validate request header.
session.ValidateRequest(requestHeader, requestType);

// return context.
return new OperationContext(requestHeader, requestType, session);
}
// validate user has permissions for additional info
session.ValidateDiagnosticInfo(requestHeader);

// return context.
return new OperationContext(requestHeader, requestType, session);
}
catch (Exception e)
{
Expand Down Expand Up @@ -584,17 +570,10 @@ private void MonitorSessions(object data)

do
{
Session[] sessions = null;

lock (m_lock)
// enumerator is thread safe
foreach (var sessionKeyValue in m_sessions)
{
sessions = new Session[m_sessions.Count];
m_sessions.Values.CopyTo(sessions, 0);
}

for (int ii = 0; ii < sessions.Length; ii++)
{
var session = sessions[ii];
Session session = sessionKeyValue.Value;
if (session.HasExpired)
{
// update diagnostics.
Expand All @@ -604,9 +583,9 @@ private void MonitorSessions(object data)
}

// raise audit event for session closed because of timeout
m_server.ReportAuditCloseSessionEvent(null, sessions[ii], "Session/Timeout");
m_server.ReportAuditCloseSessionEvent(null, session, "Session/Timeout");

m_server.CloseSession(null, sessions[ii].Id, false);
m_server.CloseSession(null, session.Id, false);
}
// if a session had no activity for the last m_minSessionTimeout milliseconds, send a keep alive event.
else if (session.ClientLastContactTime.AddMilliseconds(m_minSessionTimeout) < DateTime.UtcNow)
Expand Down Expand Up @@ -634,7 +613,7 @@ private void MonitorSessions(object data)
#region Private Fields
private readonly object m_lock = new object();
private IServerInternal m_server;
private Dictionary<NodeId, Session> m_sessions;
private ConcurrentDictionary<NodeId, Session> m_sessions;
private long m_lastSessionId;
private ManualResetEvent m_shutdownEvent;

Expand Down Expand Up @@ -789,14 +768,12 @@ public IList<Session> GetSessions()
/// <inheritdoc/>
public Session GetSession(NodeId authenticationToken)
{

Session session = null;
lock (m_lock)
// find session.
if (m_sessions.TryGetValue(authenticationToken, out Session session))
{
// find session.
m_sessions.TryGetValue(authenticationToken, out session);
return session;
}
return session;
return null;

Check warning on line 776 in Libraries/Opc.Ua.Server/Session/SessionManager.cs

View check run for this annotation

Codecov / codecov/patch

Libraries/Opc.Ua.Server/Session/SessionManager.cs#L776 

Added line #L776 was not covered by tests
}
#endregion
}
Expand Down
Loading

0 comments on commit d588ece

Please sign in to comment.