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

Simplify and improve Rude. #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
216 changes: 0 additions & 216 deletions Rude/Rude.cs

This file was deleted.

5 changes: 3 additions & 2 deletions Rude/Rude.csproj
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build" ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<PropertyGroup>
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
Expand Down Expand Up @@ -38,7 +38,8 @@
</ItemGroup>
<ItemGroup>
<Compile Include="Properties\AssemblyInfo.cs" />
<Compile Include="Rude.cs" />
<Compile Include="RudePlugin.cs" />
<Compile Include="RudeStatus.cs" />
</ItemGroup>
<Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
<ItemGroup>
Expand Down
119 changes: 119 additions & 0 deletions Rude/RudePlugin.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
using System;
using System.Linq;
using System.Threading.Tasks;
using Fancyauth.APIUtil;
using Fancyauth.API;
using System.Collections.Concurrent;

namespace Rude
{
public class RudePlugin : PluginBase
{
private readonly string[] Prefixes = { "[Rude] ", "[Ruderer] ", "[Rüdiger] " };
public const double MinRudeTime = 45;
public const double MaxRudeTime = 120;
public const double MinRudeActionTimeout = 3;
public const double MaxRudeActionTimeout = 18;
Copy link
Member

Choose a reason for hiding this comment

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

Using TimeSpan here might make this a little more obvious.


private ConcurrentDictionary<int, RudeStatus> RudeStatus = new ConcurrentDictionary<int, RudeStatus>();
private ConcurrentDictionary<int, RudeStatus> RudeActorStatus = new ConcurrentDictionary<int, RudeStatus>();

private Random rand = new Random();
Copy link
Member

Choose a reason for hiding this comment

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

The primary reason why all code in fancyauth everywhere either instantiates a new Random for every method invocation or just uses System.Security.Cryptography.RandomNumberGenerator right away is that Random is not thread-safe!

(Also, is this really called rand? Please...)


[ContextCallback("Rude")]
public async Task RudeUser(IUser actor, IUserShim targetShim)
{
var target = await targetShim.Load();
EnsureRudeStatus(target.UserId);
EnsureRudeStatus(actor.UserId);

var rude = RudeStatus[target.UserId];
var rudeActor = RudeActorStatus[actor.UserId];

if (rudeActor.RudeLevel > 0)
Copy link
Member

@main-- main-- Jan 19, 2017

Choose a reason for hiding this comment

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

Did you even stop and think about this for a second? Because unless I'm dumb this is utterly broken: The first level here means that your rude is on cooldown and lasts for the cooldown period. But as implemented, attempting to rude with your rude on cooldown adds additional levels for the full rude duration as cooldown. Also this is completely independent and never actually shows up anywhere, wtf.

{
var actorRude = RudeStatus[actor.UserId];

// You shouldn't be able to use this to reset yourself
if (rudeActor.RudeLevel < Prefixes.Length)
actorRude.RaiseRudeLevel(RandomTimespan(MinRudeTime, MaxRudeTime));

return;
}

rudeActor.RaiseRudeLevel(RandomTimespan(MinRudeActionTimeout, MaxRudeActionTimeout));
rude.RaiseRudeLevel(RandomTimespan(MinRudeTime, MaxRudeTime));

if (rude.RudeLevel > Prefixes.Length)
{
rude.Reset();
await target.Kick("You have been too rude.");
Copy link
Member

Choose a reason for hiding this comment

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

FYI in the context of guests this leaks memory as every new guest has a new userid. The best solution would be to write this to the database but in any case, guests' data has to be freed (for members keeping it is acceptable but still not pretty).

}
else
{
target.Name = GetUserName(target);
await target.SaveChanges();
}
}

public override async Task OnUserConnected(IUser user)
{
user.Name = GetUserName(user);
await user.SaveChanges();
}

private void EnsureRudeStatus(int userId)
Copy link
Member

Choose a reason for hiding this comment

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

tbh this whole function is ill-designed

because let's face it are you ever going to call this and then not use a RudeStatus?

Just call GetOrAdd on both and return them as out parameters or something. AFAICS there is no removal code so it's no big deal but if there was any, you would have to do this because your code is racy (A:ensure, B:remove, A:use).

{
if (!RudeStatus.ContainsKey(userId))
Copy link
Member

Choose a reason for hiding this comment

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

This check is pointless. Not only is it TOCTTOU-racy but it also accomplishes nothing at all - TryAdd already does this for you.

{
RudeStatus.TryAdd(userId, new RudeStatus
{
OnRudeLevelDecrease = async () =>
{
var users = await Server.GetOnlineUsers();
var user = users.SingleOrDefault(a => a.UserId == userId);
if (user == null)
return;

user.Name = GetUserName(user);
await user.SaveChanges();
}
});
}

if (!RudeActorStatus.ContainsKey(userId))
Copy link
Member

Choose a reason for hiding this comment

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

same

{
RudeActorStatus.TryAdd(userId, new RudeStatus
{
OnRudeLevelDecrease = () => { }
Copy link
Member

Choose a reason for hiding this comment

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

My opinion stands. Reusing this data structure for cooldowns feels very bolted-on so this callback still carries no value IMO. The class's one big feature is the delay mechanism where you can run code after a timeout and abstracting that might even make sense if you do it on a general layer (where it's no longer specific to rude) but that still doesn't give you a valid reason to use it here and without that, it's kinda pointless.

});
}
}

private string GetUserName(IUser user)
{
EnsureRudeStatus(user.UserId);

string name = user.Name;

// Remove potential old prefix
var startsWith = Prefixes.SingleOrDefault(a => name.StartsWith(a));
if (startsWith != null)
name = name.Substring(startsWith.Length);

var level = RudeStatus[user.UserId].RudeLevel;

if (level > 0)
return Prefixes[level - 1] + name;


return name;
}

private TimeSpan RandomTimespan(double minMinues, double maxMinutes)
{
return TimeSpan.FromMinutes(minMinues + rand.NextDouble() * (maxMinutes - minMinues));
}
}
}

Loading