From b57ace7888db78a655a00a277e7eb5c4a4eba294 Mon Sep 17 00:00:00 2001 From: Ionut Andrei Oanca Date: Mon, 30 Nov 2020 10:03:42 +0100 Subject: [PATCH] Address requested changes from review --- .../SyncPlay/GroupController.cs | 20 +---- .../SyncPlay/SyncPlayManager.cs | 75 ++++++++++++------- 2 files changed, 48 insertions(+), 47 deletions(-) diff --git a/Emby.Server.Implementations/SyncPlay/GroupController.cs b/Emby.Server.Implementations/SyncPlay/GroupController.cs index dc262f1cfc..16acae99e8 100644 --- a/Emby.Server.Implementations/SyncPlay/GroupController.cs +++ b/Emby.Server.Implementations/SyncPlay/GroupController.cs @@ -188,19 +188,6 @@ namespace Emby.Server.Implementations.SyncPlay }; } - /// - /// Checks if a given user can access a given item, that is, the user has access to a folder where the item is stored. - /// - /// The user. - /// The item. - /// true if the user can access the item, false otherwise. - private bool HasAccessToItem(User user, BaseItem item) - { - var collections = _libraryManager.GetCollectionFolders(item) - .Select(folder => folder.Id.ToString("N", CultureInfo.InvariantCulture)); - return collections.Intersect(user.GetPreference(PreferenceKind.EnabledFolders)).Any(); - } - /// /// Checks if a given user can access all items of a given queue, that is, /// the user has the required minimum parental access and has access to all required folders. @@ -219,12 +206,7 @@ namespace Emby.Server.Implementations.SyncPlay foreach (var itemId in queue) { var item = _libraryManager.GetItemById(itemId); - if (user.MaxParentalAgeRating.HasValue && item.InheritedParentalRatingValue > user.MaxParentalAgeRating) - { - return false; - } - - if (!user.HasPermission(PermissionKind.EnableAllFolders) && !HasAccessToItem(user, item)) + if (!item.IsVisibleStandalone(user)) { return false; } diff --git a/Emby.Server.Implementations/SyncPlay/SyncPlayManager.cs b/Emby.Server.Implementations/SyncPlay/SyncPlayManager.cs index fbd3c3cfb2..7e1f24f8c2 100644 --- a/Emby.Server.Implementations/SyncPlay/SyncPlayManager.cs +++ b/Emby.Server.Implementations/SyncPlay/SyncPlayManager.cs @@ -56,11 +56,17 @@ namespace Emby.Server.Implementations.SyncPlay /// /// Lock used for accessing any group. /// + /// + /// Always lock before and before locking on any . + /// private readonly object _groupsLock = new object(); /// /// Lock used for accessing the session-to-group map. /// + /// + /// Always lock after and before locking on any . + /// private readonly object _mapsLock = new object(); private bool _disposed = false; @@ -259,7 +265,12 @@ namespace Emby.Server.Implementations.SyncPlay return; } - var group = FindJoinedGroup(session); + IGroupController group; + lock (_mapsLock) + { + group = FindJoinedGroup(session); + } + if (group == null) { _logger.LogWarning("Session {SessionId} does not belong to any group.", session.Id); @@ -295,7 +306,12 @@ namespace Emby.Server.Implementations.SyncPlay { var session = e.SessionInfo; - Guid groupId = FindJoinedGroupId(session); + Guid groupId = Guid.Empty; + lock (_mapsLock) + { + groupId = FindJoinedGroupId(session); + } + if (groupId.Equals(Guid.Empty)) { return; @@ -308,33 +324,36 @@ namespace Emby.Server.Implementations.SyncPlay /// /// Checks if a given session has joined a group. /// + /// + /// Method is not thread-safe, external locking on is required. + /// /// The session. /// true if the session has joined a group, false otherwise. private bool IsSessionInGroup(SessionInfo session) { - lock (_mapsLock) - { - return _sessionToGroupMap.ContainsKey(session.Id); - } + return _sessionToGroupMap.ContainsKey(session.Id); } /// /// Gets the group joined by the given session, if any. /// + /// + /// Method is not thread-safe, external locking on is required. + /// /// The session. /// The group. private IGroupController FindJoinedGroup(SessionInfo session) { - lock (_mapsLock) - { - _sessionToGroupMap.TryGetValue(session.Id, out var group); - return group; - } + _sessionToGroupMap.TryGetValue(session.Id, out var group); + return group; } /// /// Gets the group identifier joined by the given session, if any. /// + /// + /// Method is not thread-safe, external locking on is required. + /// /// The session. /// The group identifier if the session has joined a group, an empty identifier otherwise. private Guid FindJoinedGroupId(SessionInfo session) @@ -345,6 +364,9 @@ namespace Emby.Server.Implementations.SyncPlay /// /// Maps a session to a group. /// + /// + /// Method is not thread-safe, external locking on is required. + /// /// The session. /// The group. /// Thrown when the user is in another group already. @@ -355,20 +377,20 @@ namespace Emby.Server.Implementations.SyncPlay throw new InvalidOperationException("Session is null!"); } - lock (_mapsLock) + if (IsSessionInGroup(session)) { - if (IsSessionInGroup(session)) - { - throw new InvalidOperationException("Session in other group already!"); - } - - _sessionToGroupMap[session.Id] = group ?? throw new InvalidOperationException("Group is null!"); + throw new InvalidOperationException("Session in other group already!"); } + + _sessionToGroupMap[session.Id] = group ?? throw new InvalidOperationException("Group is null!"); } /// /// Unmaps a session from a group. /// + /// + /// Method is not thread-safe, external locking on is required. + /// /// The session. /// The group. /// Thrown when the user is not found in the specified group. @@ -384,18 +406,15 @@ namespace Emby.Server.Implementations.SyncPlay throw new InvalidOperationException("Group is null!"); } - lock (_mapsLock) + if (!IsSessionInGroup(session)) { - if (!IsSessionInGroup(session)) - { - throw new InvalidOperationException("Session not in any group!"); - } + throw new InvalidOperationException("Session not in any group!"); + } - _sessionToGroupMap.Remove(session.Id, out var tempGroup); - if (!tempGroup.GroupId.Equals(group.GroupId)) - { - throw new InvalidOperationException("Session was in wrong group!"); - } + _sessionToGroupMap.Remove(session.Id, out var tempGroup); + if (!tempGroup.GroupId.Equals(group.GroupId)) + { + throw new InvalidOperationException("Session was in wrong group!"); } }