Skip to content
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

Working on caching and tokens #4001

Merged
merged 60 commits into from
Nov 10, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
05a9c42
Working on caching an tokens
jtkech Aug 5, 2019
5c0c4b8
wip
jtkech Aug 5, 2019
1a7affc
wip
jtkech Aug 7, 2019
e0bc747
wip.
jtkech Aug 8, 2019
b086ae4
Some comments
jtkech Aug 9, 2019
db21825
Invalidates the cache at the end of the transaction, right now only d…
jtkech Aug 9, 2019
a05dd5f
DeferredSignalToken to invalidate caches e.g after the session is com…
jtkech Aug 10, 2019
6e81fc3
Minor changes
jtkech Aug 10, 2019
9fb2c29
Little updates for some syncing between the caching and async branches.
jtkech Aug 11, 2019
a50d3bf
Ensures that a deferred signal for a given key is sent once at the en…
jtkech Aug 11, 2019
a14c6a7
Re-introduces and uses the DeferredSignalToken extension helper.
jtkech Aug 11, 2019
4a65c7d
2 places where the token was still get after consuming data.
jtkech Aug 18, 2019
e9180d8
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Aug 18, 2019
34c5b50
Final version of "ContentDefinitionManager" here because we keep it n…
jtkech Aug 21, 2019
05e85ba
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Aug 21, 2019
eb046aa
RoleClaims thread safety
jtkech Aug 22, 2019
fdc4742
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Aug 22, 2019
1335f17
User thread safety
jtkech Aug 23, 2019
3d6ec9d
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Sep 3, 2019
b1a1a96
Update after merging dev
jtkech Sep 4, 2019
adbb18b
Revertd changes related to Users and Roles.
jtkech Sep 4, 2019
6564933
Missing reverted update.
jtkech Sep 4, 2019
2a29015
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Sep 9, 2019
521eaec
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Sep 12, 2019
24e5bb3
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Sep 16, 2019
907ce5b
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Sep 17, 2019
ada4684
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Oct 15, 2019
6d498b9
Updates after merging dev
jtkech Oct 15, 2019
c73d921
Minor change to restart the CI
jtkech Oct 15, 2019
56fa97e
Idem.
jtkech Oct 15, 2019
6f04b8c
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Oct 16, 2019
fbfdc62
wip
jtkech Oct 17, 2019
700d0ea
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Oct 18, 2019
4b6e37e
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Oct 18, 2019
c728b3b
Re-introduce roles immutability
jtkech Oct 20, 2019
6e5bd58
Minor changes
jtkech Oct 21, 2019
514e86a
Get / load pattern for AdminMenu through #4612 keeping other caching …
jtkech Oct 23, 2019
1f1080a
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Oct 24, 2019
6b18fbb
Get / Load pattern for Roles.
jtkech Oct 24, 2019
aaa9787
Don't save default value and refresh the cache in an isolated scope.
jtkech Oct 24, 2019
fd1321c
Uses sesion.Detach()
jtkech Oct 25, 2019
f46d60b
Get / Load pattern for Templates.
jtkech Oct 25, 2019
a4dd07c
Load / Get patter for SiteSettings.
jtkech Oct 26, 2019
356f361
Get / Load pattern for Queries.
jtkech Oct 26, 2019
a4431c0
Revert some previous changes no more required with the new pattern.
jtkech Oct 26, 2019
0de3de7
Get / Load pattern for BackgroundTaskSettings.
jtkech Oct 26, 2019
f9d2a8f
Get / Load pattern for Layers.
jtkech Oct 26, 2019
07e70a3
ISessionHelper to LOAD for update / GET for caching.
jtkech Oct 28, 2019
e4d92cf
Use SessionHelper for SiteService
jtkech Oct 28, 2019
390ab72
Use SessionHelper for AdminMenuService.
jtkech Oct 28, 2019
20184a6
Use SessionHelper for other servies.
jtkech Oct 28, 2019
8153186
Wip on GET / LOAD pattern for content definitions
jtkech Oct 29, 2019
b164c14
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Oct 29, 2019
a86671a
GET / LOAD pattern for types / parts definitions.
jtkech Oct 29, 2019
a1f7eb1
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Oct 29, 2019
2ec02a5
Revert testing code, to be continued maybe in another PR.
jtkech Oct 30, 2019
9944c2b
Prevents a deferred task lambda from holding a ref on a scoped service.
jtkech Nov 3, 2019
948a49e
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Nov 8, 2019
ce3e29d
Merge remote-tracking branch 'origin/dev' into jtkech/caching
jtkech Nov 9, 2019
54c75c0
Little formatting
jtkech Nov 10, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
using Microsoft.Extensions.Localization;
using Microsoft.Extensions.Logging;
using OrchardCore.Environment.Cache;
using OrchardCore.Environment.Shell.Scope;
using OrchardCore.Modules;
using OrchardCore.Roles.Models;
using OrchardCore.Security;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Immutable;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.Extensions.Localization;
Expand Down Expand Up @@ -39,7 +40,7 @@ public async Task CreateUserAsync()

var valid = true;

await _userService.CreateUserAsync(new User { UserName = UserName, Email = Email, RoleNames = roleNames, EmailConfirmed = true }, Password, (key, message) =>
await _userService.CreateUserAsync(new User { UserName = UserName, Email = Email, RoleNames = roleNames.ToImmutableArray(), EmailConfirmed = true }, Password, (key, message) =>
{
valid = false;
Context.Output.WriteLine(message);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Immutable;
using System.Threading.Tasks;
using OrchardCore.Setup.Events;
using OrchardCore.Users.Models;
Expand Down Expand Up @@ -33,7 +34,7 @@ Action<string, string> reportError
{
UserName = userName,
Email = email,
RoleNames = new string[] { "Administrator" },
RoleNames = ImmutableArray.Create("Administrator"),
EmailConfirmed = true
};

Expand Down
10 changes: 5 additions & 5 deletions src/OrchardCore/OrchardCore.Users.Core/Models/User.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using Microsoft.AspNetCore.Identity;
using OrchardCore.Entities;

Expand All @@ -15,12 +15,12 @@ public class User : Entity, IUser
public string SecurityStamp { get; set; }
public bool EmailConfirmed { get; set; }
public string ResetToken { get; set; }
public IList<string> RoleNames { get; set; } = new List<string>();
public IList<UserClaim> UserClaims { get; set; } = new List<UserClaim>();
public IList<UserLoginInfo> LoginInfos { get; set; } = new List<UserLoginInfo>();
public ImmutableArray<string> RoleNames { get; set; } = ImmutableArray<string>.Empty;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Is a User object used in concurrent threads?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i remember, there are some places where we mutate it and i checked that we act on a same shared instance.

Let me some time to retrieve an example

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g in UserStore.AddToRoleAsync() line 332

        ((User)user).RoleNames.Add(roleName);

That becomes here

        ((User)user).RoleNames = ((User)user).RoleNames.Add(roleName);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But only one thread accesses this user instance, so the list is thread-safe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes normally but just tried 2 concurrent editing (by using a break point) of the same user, i could see that the 2 concurrent thread are mutating the same user instance and then its roles collection.

The other problem is when we are using / enumerating on e.g roleNames while mutating it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Where are we mutating and iterating at the same time, if not on two different thread.

just tried 2 concurrent editing

This is worrying. Can you explain how that happens?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UserManager is scoped, it resolves UserStore which is scoped too, which uses Session to load a user. In the end I don't understand how two thread would access the same user instance, unless we have some wrong parallel code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay let me check again

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, just retried, you're right there are 2 different instances, i don't know what i did, maybe too tired ;) These were some of the last changes i did as for others singletons and without doing the above test, indeed there are all scoped services, i'm stupid.

Thanks, i will revert all the changes around user and roles.

Copy link
Member Author

@jtkech jtkech Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, i don't know why i did here the same changes as for other services but using a singleton cache.

Sorry, i reverted the changes related to users and roles.

public ImmutableArray<UserClaim> UserClaims { get; set; } = ImmutableArray<UserClaim>.Empty;
public ImmutableArray<UserLoginInfo> LoginInfos { get; set; } = ImmutableArray<UserLoginInfo>.Empty;

public override string ToString()
{
{
return UserName;
}
}
Expand Down
53 changes: 37 additions & 16 deletions src/OrchardCore/OrchardCore.Users.Core/Services/UserStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ public async Task AddToRoleAsync(IUser user, string normalizedRoleName, Cancella
throw new InvalidOperationException($"Role {normalizedRoleName} does not exist.");
}

((User)user).RoleNames.Add(roleName);
((User)user).RoleNames = ((User)user).RoleNames.Add(roleName);
}

public async Task RemoveFromRoleAsync(IUser user, string normalizedRoleName, CancellationToken cancellationToken)
Expand All @@ -347,7 +347,7 @@ public async Task RemoveFromRoleAsync(IUser user, string normalizedRoleName, Can
throw new InvalidOperationException($"Role {normalizedRoleName} does not exist.");
}

((User)user).RoleNames.Remove(roleName);
((User)user).RoleNames = ((User)user).RoleNames.Remove(roleName);
}

public Task<IList<string>> GetRolesAsync(IUser user, CancellationToken cancellationToken)
Expand Down Expand Up @@ -401,9 +401,11 @@ public Task AddLoginAsync(IUser user, UserLoginInfo login, CancellationToken can
}

if (((User)user).LoginInfos.Any(i => i.LoginProvider == login.LoginProvider))
{
throw new InvalidOperationException($"Provider {login.LoginProvider} is already linked for {user.UserName}");
}

((User)user).LoginInfos.Add(login);
((User)user).LoginInfos = ((User)user).LoginInfos.Add(login);

return Task.CompletedTask;
}
Expand All @@ -430,15 +432,12 @@ public Task RemoveLoginAsync(IUser user, string loginProvider, string providerKe
throw new ArgumentNullException(nameof(user));
}

var externalLogins = ((User)user).LoginInfos;
if (externalLogins != null)
var item = ((User)user).LoginInfos.FirstOrDefault(c => c.LoginProvider == loginProvider && c.ProviderKey == providerKey);
if (item != null)
{
var item = externalLogins.FirstOrDefault(c => c.LoginProvider == loginProvider && c.ProviderKey == providerKey);
if (item != null)
{
externalLogins.Remove(item);
}
((User)user).LoginInfos = ((User)user).LoginInfos.Remove(item);
}

return Task.CompletedTask;
}

Expand All @@ -458,13 +457,18 @@ public Task<IList<Claim>> GetClaimsAsync(IUser user, CancellationToken cancellat
public Task AddClaimsAsync(IUser user, IEnumerable<Claim> claims, CancellationToken cancellationToken)
{
if (user == null)
{
throw new ArgumentNullException(nameof(user));
}

if (claims == null)
{
throw new ArgumentNullException(nameof(claims));
}

foreach (var claim in claims)
{
((User)user).UserClaims.Add(new UserClaim{ClaimType = claim.Type, ClaimValue = claim.Value});
((User)user).UserClaims = ((User)user).UserClaims.Add(new UserClaim { ClaimType = claim.Type, ClaimValue = claim.Value });
}

return Task.CompletedTask;
Expand All @@ -473,16 +477,24 @@ public Task AddClaimsAsync(IUser user, IEnumerable<Claim> claims, CancellationTo
public Task ReplaceClaimAsync(IUser user, Claim claim, Claim newClaim, CancellationToken cancellationToken)
{
if (user == null)
{
throw new ArgumentNullException(nameof(user));
}

if (claim == null)
{
throw new ArgumentNullException(nameof(claim));
}

if (newClaim == null)
{

throw new ArgumentNullException(nameof(newClaim));
}

foreach (var userClaim in ((User) user).UserClaims.Where(uc => uc.ClaimValue == claim.Value && uc.ClaimType == claim.Type))
foreach (var userClaim in ((User)user).UserClaims.Where(uc => uc.ClaimValue == claim.Value && uc.ClaimType == claim.Type))
{
userClaim.ClaimValue = newClaim.Value;
userClaim.ClaimType = newClaim.Type;
((User)user).UserClaims = ((User)user).UserClaims.Replace(userClaim, new UserClaim { ClaimType = newClaim.Type, ClaimValue = newClaim.Value });
}

return Task.CompletedTask;
Expand All @@ -491,14 +503,21 @@ public Task ReplaceClaimAsync(IUser user, Claim claim, Claim newClaim, Cancellat
public Task RemoveClaimsAsync(IUser user, IEnumerable<Claim> claims, CancellationToken cancellationToken)
{
if (user == null)
{
throw new ArgumentNullException(nameof(user));
}

if (claims == null)
{
throw new ArgumentNullException(nameof(claims));
}

foreach (var claim in claims)
{
foreach (var userClaim in ((User)user).UserClaims.Where(uc => uc.ClaimValue == claim.Value && uc.ClaimType == claim.Type).ToList())
((User)user).UserClaims.Remove(userClaim);
{
((User)user).UserClaims = ((User)user).UserClaims.Remove(userClaim);
}
}

return Task.CompletedTask;
Expand All @@ -507,8 +526,10 @@ public Task RemoveClaimsAsync(IUser user, IEnumerable<Claim> claims, Cancellatio
public async Task<IList<IUser>> GetUsersForClaimAsync(Claim claim, CancellationToken cancellationToken)
{
if (claim == null)
{
throw new ArgumentNullException(nameof(claim));

}

var users = await _session.Query<User, UserByClaimIndex>(uc => uc.ClaimType == claim.Type && uc.ClaimValue == claim.Value).ListAsync();

return users.Cast<IUser>().ToList();
Expand Down