Skip to content

Commit 498a8b7

Browse files
rozniakrozniak
authored
Add AD Caching - Fixes #872 (#873)
* Performance improvements via caching of Principals and PrincipalContexts This commit brings some performance improvements by implementing some basic caching of Principal and PrincipalContext objects instead of init'ing them every time. Saves about 2 seconds per init from my usage so far. Expiry right now is hard-coded to 10 minutes, should really shift this to a web.config value. * Performance improvements via caching of PrincipalSearchResults This commit provides huge performance gains by caching the results of calls to GroupPrincipal.GetMembers(true) by enumerating the PrincipalSearchResults object and storing it. Normally this class lazy loads the results, so the enum is triggered up front and stored for later use. On my use case this sped up authorization calls from 10+ seconds to under a second. Like the previous commit, the cache is set to expire after 10 minutes - I will endeavour to move this out into web.config. * Add configuration keys for AD cache expiry durations This commit shifts the hard-coded cache expiry for Active Directory objects of 10 minutes into configurable options. The keys are: ActiveDirectoryGroupQueryCacheExpiry ActiveDirectoryPrincipalCacheExpiry They will be parsed into TimeSpan objects by ConfigurationHelper.ParseTimeSpanOrDefault (defaulting to 10 minutes). * Cleanup for PR This commit addresses some notes in the PR: - Locks sections have been fixed up and slightly improved in terms of readability - Implement IDisposable on cache objects, most important on ADCachedSearchResult as it will dispose the enumerated principals - Formatting tidy-up, braces * Amendments for IDisposable implementations on cache objects Replace Disposing with _Disposing and use nameof when retrieving property name as part of throwing ObjectDisposedExceptions. * Correct dispose exception throw, code to use corefx style - throw new ObjectDisposedException refers to the class rather than the property - update private and private static member identifiers to follow corefx style conventions
1 parent f44c9b2 commit 498a8b7

File tree

8 files changed

+497
-118
lines changed

8 files changed

+497
-118
lines changed

‎Bonobo.Git.Server/Bonobo.Git.Server.csproj

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,8 @@
284284
<Compile Include="Controllers\ValidationController.cs" />
285285
<Compile Include="Data\ADBackend.cs" />
286286
<Compile Include="Data\ADBackendStore.cs" />
287+
<Compile Include="Data\ADCachedPrincipal.cs" />
288+
<Compile Include="Data\ADCachedSearchResult.cs" />
287289
<Compile Include="Data\ADRepositoryRepository.cs" />
288290
<Compile Include="Data\ADTeamRepository.cs" />
289291
<Compile Include="Data\DatabaseResetManager.cs" />
@@ -318,6 +320,7 @@
318320
<Compile Include="Git\GitService\ReceivePackHook\ReceivePackCommitSignature.cs" />
319321
<Compile Include="Git\GitService\ReplicatingStream.cs" />
320322
<Compile Include="Helpers\ADHelper.cs" />
323+
<Compile Include="Helpers\ConfigurationHelper.cs" />
321324
<Compile Include="Helpers\MembershipHelper.cs" />
322325
<Compile Include="Helpers\RepositoryCommitModelHelpers.cs" />
323326
<Compile Include="Models\RepositoryCommitTitle.cs" />

‎Bonobo.Git.Server/Data/ADBackend.cs

Lines changed: 30 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -163,23 +163,21 @@ private void UpdateUsers()
163163
{
164164
try
165165
{
166-
GroupPrincipal group;
167-
using (var pc = ADHelper.GetMembersGroup(out group))
166+
GroupPrincipal group;
167+
PrincipalContext pc = ADHelper.GetMembersGroup(out group);
168+
169+
foreach (Guid Id in Users.Select(x => x.Id).Where(x => ADHelper.GetUserPrincipal(x) == null))
170+
{
171+
Users.Remove(Id);
172+
}
173+
174+
foreach (string username in ADHelper.GetGroupMembers(group).OfType<UserPrincipal>().Select(x => x.UserPrincipalName).Where(x => x != null))
168175
{
169-
foreach (Guid Id in Users.Select(x => x.Id).Where(x => ADHelper.GetUserPrincipal(x) == null))
176+
UserPrincipal principal = ADHelper.GetUserPrincipal(username);
177+
UserModel user = GetUserModelFromPrincipal(principal);
178+
if (user != null)
170179
{
171-
Users.Remove(Id);
172-
}
173-
foreach (string username in group.GetMembers(true).OfType<UserPrincipal>().Select(x => x.UserPrincipalName).Where(x => x != null))
174-
{
175-
using (var principal = ADHelper.GetUserPrincipal(username))
176-
{
177-
UserModel user = GetUserModelFromPrincipal(principal);
178-
if (user != null)
179-
{
180-
Users.AddOrUpdate(user);
181-
}
182-
}
180+
Users.AddOrUpdate(user);
183181
}
184182
}
185183
}
@@ -207,17 +205,16 @@ private void UpdateTeams()
207205
try
208206
{
209207
GroupPrincipal group;
210-
using (var pc = ADHelper.GetPrincipalGroup(groupName, out group))
208+
PrincipalContext pc = ADHelper.GetPrincipalGroup(groupName, out group);
209+
TeamModel teamModel = new TeamModel()
211210
{
212-
TeamModel teamModel = new TeamModel() {
213-
Id = group.Guid.Value,
214-
Description = group.Description,
215-
Name = teamName,
216-
Members = group.GetMembers(true).Select(x => MembershipService.GetUserModel(x.Guid.Value)).Where(o => o != null).ToArray()
217-
};
218-
Teams.AddOrUpdate(teamModel);
219-
Log.Verbose("AD: Updated team {TeamName} OK", teamName);
220-
}
211+
Id = group.Guid.Value,
212+
Description = group.Description,
213+
Name = teamName,
214+
Members = ADHelper.GetGroupMembers(group).Select(x => MembershipService.GetUserModel(x.Guid.Value)).Where(o => o != null).ToArray()
215+
};
216+
Teams.AddOrUpdate(teamModel);
217+
Log.Verbose("AD: Updated team {TeamName} OK", teamName);
221218
}
222219
catch (Exception ex)
223220
{
@@ -240,18 +237,16 @@ private void UpdateRoles()
240237
try
241238
{
242239
GroupPrincipal group;
243-
using (var pc = ADHelper.GetPrincipalGroup(groupName, out group))
240+
PrincipalContext pc = ADHelper.GetPrincipalGroup(groupName, out group);
241+
RoleModel roleModel = new RoleModel
244242
{
245-
RoleModel roleModel = new RoleModel
246-
{
247-
Id = group.Guid.Value,
248-
Name = roleName,
249-
Members = group.GetMembers(true).Where(x => x is UserPrincipal).Select(x => x.Guid.Value)
243+
Id = group.Guid.Value,
244+
Name = roleName,
245+
Members = ADHelper.GetGroupMembers(group).Where(x => x is UserPrincipal).Select(x => x.Guid.Value)
250246
.ToArray()
251-
};
252-
Roles.AddOrUpdate(roleModel);
253-
Log.Verbose("AD: Updated role {RoleName} OK", roleName);
254-
}
247+
};
248+
Roles.AddOrUpdate(roleModel);
249+
Log.Verbose("AD: Updated role {RoleName} OK", roleName);
255250
}
256251
catch (Exception ex)
257252
{

‎Bonobo.Git.Server/Data/ADCachedPrincipal.cs

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
using System;
2+
using System.Collections.Generic;
3+
using System.DirectoryServices.AccountManagement;
4+
using System.Linq;
5+
using System.Web;
6+
7+
namespace Bonobo.Git.Server.Data
8+
{
9+
public class ADCachedPrincipal : IDisposable
10+
{
11+
public DateTime CacheTime { get; private set; }
12+
public Principal Principal { get; private set; }
13+
public PrincipalContext PrincipalContext { get; private set; }
14+
15+
16+
private bool _disposing;
17+
18+
19+
public ADCachedPrincipal(PrincipalContext pc, Principal principal)
20+
{
21+
_disposing = false;
22+
CacheTime = DateTime.UtcNow;
23+
Principal = principal;
24+
PrincipalContext = pc;
25+
}
26+
27+
28+
public void Dispose()
29+
{
30+
if (_disposing)
31+
{
32+
throw new ObjectDisposedException(nameof(ADCachedPrincipal));
33+
}
34+
35+
_disposing = true;
36+
Principal.Dispose();
37+
}
38+
}
39+
}

‎Bonobo.Git.Server/Data/ADCachedSearchResult.cs

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
using System;
2+
using System.Collections.Generic;
3+
using System.DirectoryServices.AccountManagement;
4+
using System.Linq;
5+
using System.Web;
6+
7+
namespace Bonobo.Git.Server.Data
8+
{
9+
public class ADCachedSearchResult : IDisposable
10+
{
11+
public DateTime CacheTime { get; private set; }
12+
public IList<Principal> Principals { get; private set; }
13+
public PrincipalSearchResult<Principal> SearchResults { get; private set; }
14+
15+
16+
private bool _disposing;
17+
18+
19+
public ADCachedSearchResult(PrincipalSearchResult<Principal> searchResults)
20+
{
21+
_disposing = false;
22+
CacheTime = DateTime.UtcNow;
23+
SearchResults = searchResults;
24+
25+
// Search results are lazy loaded, we should enum them now so the cache is
26+
// effective
27+
//
28+
var principals = new List<Principal>();
29+
30+
foreach (Principal principal in SearchResults)
31+
{
32+
principals.Add(principal);
33+
}
34+
35+
Principals = principals.AsReadOnly();
36+
}
37+
38+
39+
public void Dispose()
40+
{
41+
if (_disposing)
42+
{
43+
throw new ObjectDisposedException(nameof(ADCachedSearchResult));
44+
}
45+
46+
_disposing = true;
47+
48+
foreach (Principal principal in Principals)
49+
{
50+
principal.Dispose();
51+
}
52+
53+
SearchResults.Dispose();
54+
}
55+
}
56+
}

‎Bonobo.Git.Server/Data/Update/ADBackend/UpdateADBackend.cs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -202,10 +202,9 @@ private static void UpdateRoles(string dir, Dictionary<string, Models.UserModel>
202202
try
203203
{
204204
GroupPrincipal group;
205-
using (var pc = ADHelper.GetPrincipalGroup(ActiveDirectorySettings.TeamNameToGroupNameMapping[team.Name], out group))
206-
{
207-
newteam.Id = group.Guid.Value;
208-
}
205+
PrincipalContext pc = ADHelper.GetPrincipalGroup(ActiveDirectorySettings.TeamNameToGroupNameMapping[team.Name], out group);
206+
207+
newteam.Id = group.Guid.Value;
209208
}
210209
catch (Exception ex)
211210
{

0 commit comments

Comments
 (0)