Memory leak in userMap? Essentials User Cache issues?
L4BORG opened this issue ยท 27 comments
I would like to address this again since @vemacs already closed #814 however and the issue is still there, if this is not Essentials issue and is caused by depending plugins it would be nice to know if anyone has any ideas what is going on, because something is causing memory leaks in essentials.
I've already confirmed this with:
- https://www.spigotmc.org/resources/eggwars-villagers-multiarena-bungee-generators-configurable.14476/
- https://github.com/walrusone/SkyWarsReloaded
and tried:
max-user-cache-count: 10000
but it didn't help, besides restarting the server, running /ess reload twice would usually free that memory, besides that I have no idea what is going on...
Also I've tried purging user files (ess and .dat files) ... it didn't help.
dmulloy2/ChestShop-3#10 - is this related by any chance?
Maybe something is wrong with Ess player cache? I'm guessing it happens when a player would change his username (but UUID would stay the same) not sure how cache would react on that. I doubt this is what's causing memory leaks, however I think something is very wrong with current cache implementation.
Here we go again:
[16:43:27] [Server thread/ERROR]: Could not pass event CurrencyAddEvent to ChestShop v3.8.13
org.bukkit.event.EventException
at org.bukkit.plugin.java.JavaPluginLoader$1.execute(JavaPluginLoader.java:302) ~[patched.jar:git-TacoSpigot-"65fd35f"]
at co.aikar.timings.TimedEventExecutor.execute(TimedEventExecutor.java:74) ~[patched.jar:git-TacoSpigot-"65fd35f"]
at org.bukkit.plugin.RegisteredListener.callEvent(RegisteredListener.java:62) ~[patched.jar:git-TacoSpigot-"65fd35f"]
at org.bukkit.plugin.SimplePluginManager.fireEvent(SimplePluginManager.java:501) [patched.jar:git-TacoSpigot-"65fd35f"]
at org.bukkit.plugin.SimplePluginManager.callEvent(SimplePluginManager.java:486) [patched.jar:git-TacoSpigot-"65fd35f"]
at com.Acrobot.ChestShop.ChestShop.callEvent(ChestShop.java:404) [ChestShop.jar:?]
at com.Acrobot.ChestShop.Listeners.PostTransaction.EconomicModule.onBuyTransaction(EconomicModule.java:28) [ChestShop.jar:?]
at sun.reflect.GeneratedMethodAccessor729.invoke(Unknown Source) ~[?:?]
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_102]
at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_102]
at org.bukkit.plugin.java.JavaPluginLoader$1.execute(JavaPluginLoader.java:300) [patched.jar:git-TacoSpigot-"65fd35f"]
at co.aikar.timings.TimedEventExecutor.execute(TimedEventExecutor.java:74) [patched.jar:git-TacoSpigot-"65fd35f"]
at org.bukkit.plugin.RegisteredListener.callEvent(RegisteredListener.java:62) [patched.jar:git-TacoSpigot-"65fd35f"]
at org.bukkit.plugin.SimplePluginManager.fireEvent(SimplePluginManager.java:501) [patched.jar:git-TacoSpigot-"65fd35f"]
at org.bukkit.plugin.SimplePluginManager.callEvent(SimplePluginManager.java:486) [patched.jar:git-TacoSpigot-"65fd35f"]
at com.Acrobot.ChestShop.Listeners.Player.PlayerInteract.onInteract(PlayerInteract.java:111) [ChestShop.jar:?]
at sun.reflect.GeneratedMethodAccessor291.invoke(Unknown Source) ~[?:?]
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_102]
at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_102]
at org.bukkit.plugin.java.JavaPluginLoader$1.execute(JavaPluginLoader.java:300) [patched.jar:git-TacoSpigot-"65fd35f"]
at co.aikar.timings.TimedEventExecutor.execute(TimedEventExecutor.java:74) [patched.jar:git-TacoSpigot-"65fd35f"]
at org.bukkit.plugin.RegisteredListener.callEvent(RegisteredListener.java:62) [patched.jar:git-TacoSpigot-"65fd35f"]
at org.bukkit.plugin.SimplePluginManager.fireEvent(SimplePluginManager.java:501) [patched.jar:git-TacoSpigot-"65fd35f"]
at org.bukkit.plugin.SimplePluginManager.callEvent(SimplePluginManager.java:486) [patched.jar:git-TacoSpigot-"65fd35f"]
at org.bukkit.craftbukkit.v1_8_R3.event.CraftEventFactory.callPlayerInteractEvent(CraftEventFactory.java:228) [patched.jar:git-TacoSpigot-"65fd35f"]
at net.minecraft.server.v1_8_R3.PlayerInteractManager.interact(PlayerInteractManager.java:463) [patched.jar:git-TacoSpigot-"65fd35f"]
at net.minecraft.server.v1_8_R3.PlayerConnection.a(PlayerConnection.java:763) [patched.jar:git-TacoSpigot-"65fd35f"]
at net.minecraft.server.v1_8_R3.PacketPlayInBlockPlace.a(PacketPlayInBlockPlace.java:52) [patched.jar:git-TacoSpigot-"65fd35f"]
at net.minecraft.server.v1_8_R3.PacketPlayInBlockPlace.a(PacketPlayInBlockPlace.java:1) [patched.jar:git-TacoSpigot-"65fd35f"]
at net.minecraft.server.v1_8_R3.PlayerConnectionUtils$1.run(SourceFile:13) [patched.jar:git-TacoSpigot-"65fd35f"]
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [?:1.8.0_102]
at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:1.8.0_102]
at net.minecraft.server.v1_8_R3.SystemUtils.a(SourceFile:44) [patched.jar:git-TacoSpigot-"65fd35f"]
at net.minecraft.server.v1_8_R3.MinecraftServer.B(MinecraftServer.java:774) [patched.jar:git-TacoSpigot-"65fd35f"]
at net.minecraft.server.v1_8_R3.DedicatedServer.B(DedicatedServer.java:378) [patched.jar:git-TacoSpigot-"65fd35f"]
at net.minecraft.server.v1_8_R3.MinecraftServer.A(MinecraftServer.java:713) [patched.jar:git-TacoSpigot-"65fd35f"]
at net.minecraft.server.v1_8_R3.MinecraftServer.run(MinecraftServer.java:616) [patched.jar:git-TacoSpigot-"65fd35f"]
at java.lang.Thread.run(Thread.java:745) [?:1.8.0_102]
Caused by: com.google.common.util.concurrent.ExecutionError: java.lang.StackOverflowError
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2199) ~[patched.jar:git-TacoSpigot-"65fd35f"]
at com.google.common.cache.LocalCache.get(LocalCache.java:3934) ~[patched.jar:git-TacoSpigot-"65fd35f"]
at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:3938) ~[patched.jar:git-TacoSpigot-"65fd35f"]
at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:4821) ~[patched.jar:git-TacoSpigot-"65fd35f"]
at com.earth2me.essentials.UserMap.getUser(UserMap.java:111) ~[?:?]
at com.earth2me.essentials.UserMap.getUser(UserMap.java:92) ~[?:?]
at com.earth2me.essentials.Essentials.getOfflineUser(Essentials.java:585) ~[?:?]
at com.earth2me.essentials.Essentials.getUser(Essentials.java:573) ~[?:?]
at com.earth2me.essentials.api.Economy.getUserByName(Economy.java:67) ~[?:?]
at com.earth2me.essentials.api.Economy.getMoneyExact(Economy.java:85) ~[?:?]
at com.earth2me.essentials.api.Economy.add(Economy.java:148) ~[?:?]
at com.earth2me.essentials.api.Economy.add(Economy.java:141) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:146) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151) ~[?:?]
at net.milkbowl.vault.economy.plugins.Economy_Essentials.depositPlayer(Economy_Essentials.java:151
I think this is Essentials issue, causing dmulloy2/ChestShop-3#10 and #855
Also /ess reload every 1h would fix memory leak issues but not StackOverflowError issues...
You said you were using a custom version of ess that fixed the issue? Care to share?
I did? Stalker... >.<
Economy issue is not Essentials related.
Tried reverting a4871ca and the memory leak was still there, I guess it's something else.
Next thing... https://github.com/drtshock/Essentials/blob/2.x/Essentials/src/com/earth2me/essentials/UserMap.java#L178
shouldn't
public void removeUser(final String name) {
if (names == null) {
ess.getLogger().warning("Name collection is null, cannot remove user.");
return;
}
UUID uuid = names.get(name);
if (uuid != null) {
keys.remove(uuid);
users.invalidate(uuid);
}
names.remove(name);
names.remove(StringUtil.safeString(name));
}
be
public void removeUser(final String name) {
if (names == null) {
ess.getLogger().warning("Name collection is null, cannot remove user.");
return;
}
UUID uuid = names.get(name);
if (uuid != null) {
keys.remove(uuid);
users.invalidate(uuid.toString());
}
names.remove(name);
names.remove(StringUtil.safeString(name));
}
?
And any idea why UUIDs are used as String objects (and not UUID) for keys.
There are a lot of legacy methods relying on UUIDs being handled as strings, even within the class. You're right about invalidate() though.
The problem seems to be poor handling of createPlayerAccount rather than a real leak.
I've rewritten part SkyWarsReloaded so it would use Essentials Eco instead of Vault, but that memory leak is still there...
It is Essentials' fault for improperly handling createPlayerAccount, but it's not an issue with the UserMap.
Also...
is wrong according to...
?
But this shouldn't be the reason for memory leak, right?
Okay, maybe it's not an issue... https://github.com/MilkBowl/Vault/blob/master/src/net/milkbowl/vault/economy/plugins/Economy_Essentials.java#L230
Still...
And even that has most likely nothing to do with the memory leak. Even tho softValues() is there GC just won't free that memory - but only on some servers, we're running Ess and Vault on our lobbies too and we don't have issues with memory leaks there.
Calling (UserMap)
public void invalidateUser(final UUID uuid)
{
if (uuid != null)
users.invalidate(uuid.toString());
else
ess.getLogger().warning("UUID is null, cannot invalidate user cache.");
}
from lets say UserData would do the trick, ie:
public final void cleanup() {
config.cleanup();
if (config.uuid != null)
ess.getUserMap().invalidateUser(config.uuid);
}
but I don't think this is the main issue here...
And Vault...
public class VaultEco implements Method {
private Vault vault;
private Economy economy;
@Override
public Vault getPlugin() {
return this.vault;
}
@Override
public String getName() {
return this.vault.getDescription().getName();
}
public String getEconomy() {
return economy == null ? "NoEco" : economy.getName();
}
@Override
public String getLongName() {
return getName().concat(" - Economy: ").concat(getEconomy());
}
@Override
public String getVersion() {
return this.vault.getDescription().getVersion();
}
@Override
public int fractionalDigits() {
return 0;
}
@Override
public String format(double amount) {
return this.economy.format(amount);
}
@Override
public boolean hasBanks() {
return this.economy.hasBankSupport();
}
@Override
public boolean hasBank(String bank) {
return this.economy.getBanks().contains(bank);
}
@Override
public boolean hasAccount(String name) {
OfflinePlayer offlinePlayer = new OfflinePlayer(name, Essentials.getInstance().getServer());
return this.economy.hasAccount(offlinePlayer);
//return this.economy.hasAccount(name);
}
@Override
public boolean hasBankAccount(String bank, String name) {
OfflinePlayer offlinePlayer = new OfflinePlayer(name, Essentials.getInstance().getServer());
return this.economy.isBankOwner(bank, offlinePlayer).transactionSuccess() || this.economy.isBankMember(bank, offlinePlayer).transactionSuccess();
//return this.economy.isBankOwner(bank, name).transactionSuccess() || this.economy.isBankMember(bank, name).transactionSuccess();
}
@Override
public boolean createAccount(String name) {
OfflinePlayer offlinePlayer = new OfflinePlayer(name, Essentials.getInstance().getServer());
return this.economy.createPlayerAccount(offlinePlayer);
//return this.economy.createBank(name, "").transactionSuccess();
}
@Override
public boolean createAccount(String name, Double amount)
{
if (hasAccount(name))
return false;
OfflinePlayer offlinePlayer = new OfflinePlayer(name, Essentials.getInstance().getServer());
if (!this.economy.createPlayerAccount(offlinePlayer))
return false;
return this.economy.depositPlayer(offlinePlayer, amount).transactionSuccess();
}
@Override
public MethodAccount getAccount(String name) {
if (!hasAccount(name)) {
return null;
}
return new VaultAccount(name, this.economy);
}
@Override
public MethodBankAccount getBankAccount(String bank, String name) {
if (!hasBankAccount(bank, name)) {
return null;
}
return new VaultBankAccount(bank, economy);
}
@Override
public boolean isCompatible(Plugin plugin) {
try {
RegisteredServiceProvider<Economy> ecoPlugin = plugin.getServer().getServicesManager().getRegistration(net.milkbowl.vault.economy.Economy.class);
return plugin instanceof Vault && ecoPlugin != null && !ecoPlugin.getProvider().getName().equals("Essentials Economy");
} catch (LinkageError e) {
return false;
} catch (Exception e) {
return false;
}
}
@Override
public void setPlugin(Plugin plugin) {
this.vault = (Vault) plugin;
RegisteredServiceProvider<Economy> economyProvider = this.vault.getServer().getServicesManager().getRegistration(net.milkbowl.vault.economy.Economy.class);
if (economyProvider != null) {
this.economy = economyProvider.getProvider();
}
}
public class VaultAccount implements MethodAccount {
private final String name;
private final Economy economy;
public VaultAccount(String name, Economy economy) {
this.name = name;
this.economy = economy;
}
@Override
public double balance() {
OfflinePlayer offlinePlayer = new OfflinePlayer(this.name, Essentials.getInstance().getServer());
return this.economy.getBalance(offlinePlayer);
//return this.economy.getBalance(this.name);
}
@Override
public boolean set(double amount)
{
OfflinePlayer offlinePlayer = new OfflinePlayer(this.name, Essentials.getInstance().getServer());
if (!this.economy.withdrawPlayer(offlinePlayer, this.balance()).transactionSuccess()) {
//if (!this.economy.withdrawPlayer(this.name, this.balance()).transactionSuccess()) {
return false;
}
if (amount == 0) {
return true;
}
return this.economy.depositPlayer(offlinePlayer, amount).transactionSuccess();
//return this.economy.depositPlayer(this.name, amount).transactionSuccess();
}
@Override
public boolean add(double amount) {
OfflinePlayer offlinePlayer = new OfflinePlayer(this.name, Essentials.getInstance().getServer());
return this.economy.depositPlayer(offlinePlayer, amount).transactionSuccess();
//return this.economy.depositPlayer(this.name, amount).transactionSuccess();
}
@Override
public boolean subtract(double amount) {
OfflinePlayer offlinePlayer = new OfflinePlayer(this.name, Essentials.getInstance().getServer());
return this.economy.withdrawPlayer(offlinePlayer, amount).transactionSuccess();
//return this.economy.withdrawPlayer(this.name, amount).transactionSuccess();
}
@Override
public boolean multiply(double amount) {
double balance = this.balance();
return this.set(balance * amount);
}
@Override
public boolean divide(double amount) {
double balance = this.balance();
return this.set(balance / amount);
}
@Override
public boolean hasEnough(double amount) {
return (this.balance() >= amount);
}
@Override
public boolean hasOver(double amount) {
return (this.balance() > amount);
}
@Override
public boolean hasUnder(double amount) {
return (this.balance() < amount);
}
@Override
public boolean isNegative() {
return (this.balance() < 0);
}
@Override
public boolean remove() {
return this.set(0.0);
}
}
public class VaultBankAccount implements MethodBankAccount {
private final String bank;
private final Economy economy;
public VaultBankAccount(String bank, Economy economy) {
this.bank = bank;
this.economy = economy;
}
@Override
public String getBankName() {
return this.bank;
}
@Override
public int getBankId() {
return -1;
}
@Override
public double balance() {
return this.economy.bankBalance(this.bank).balance;
}
@Override
public boolean set(double amount) {
if (!this.economy.bankWithdraw(this.bank, this.balance()).transactionSuccess()) {
return false;
}
if (amount == 0) {
return true;
}
return this.economy.bankDeposit(this.bank, amount).transactionSuccess();
}
@Override
public boolean add(double amount) {
return this.economy.bankDeposit(this.bank, amount).transactionSuccess();
}
@Override
public boolean subtract(double amount) {
return this.economy.bankWithdraw(this.bank, amount).transactionSuccess();
}
@Override
public boolean multiply(double amount) {
double balance = this.balance();
return this.set(balance * amount);
}
@Override
public boolean divide(double amount) {
double balance = this.balance();
return this.set(balance / amount);
}
@Override
public boolean hasEnough(double amount) {
return (this.balance() >= amount);
}
@Override
public boolean hasOver(double amount) {
return (this.balance() > amount);
}
@Override
public boolean hasUnder(double amount) {
return (this.balance() < amount);
}
@Override
public boolean isNegative() {
return (this.balance() < 0);
}
@Override
public boolean remove() {
return this.set(0.0);
}
}
}
but I'm not very familiar Essentials and I think there should be better way of getting Essentials.getInstance().getServer() without static abuse or maybe passing it as param to VaultEco.
Nvm, forget all that, did some more testing and I think changing balance for offline players is the issue (it would load that data back into users cache - and wouldn't remove data even after player rejoined and left again).
This makes sense since SkyWars/EggWars would update players balance if the player left the server while playing...
The same thing would happen with ChestShop (CS would update bal whenever the player is online or not).
After loading data (for offline player) into users the data would stay there even if users.invalidate(uuid.toString()); is called for specific player.
And there's one more thing, /baltop would load all the data into users guava cache right away on a server with 30000 user files that would result in ~710MB of data being loaded and stored there which is 20kb per User object and that's... a lot?
IIRC with Xmx4G users cache would be capped somewhere a bit over ~40k which would most likely be around 1GB of useless data. While it would make sense to use expireAfterAccess or something like that and cache User objects that are actually used... I don't think having 1/4 of memory used for "dumb"-unused objects that are just sitting there is a good idea (same goes for storing them or having GC work on freeing that memory).
I think this was already "fixed" for /seen command awhile ago, maybe using the same approach (skip loading data into cache) with baltop wouldn't be such a bad idea...
/baltop doesn't load all filesuserdata directory into memory, nothing further. However, due to this behaviour, this also includes data which are npcs, the only way to tell if it's an npc is to load the file. However, this can easily be identified as all npcs have UUID v3.
Furthermore, if this was to be fixed by opening all files, this would increase loading times by a bunch for offline mode servers that basically have v3 for all player UUIDs.
bash: /bin/cat: Argument list too long
0
I lot I guess, in total there are a bit over 105000 files in plugins/Essentials/userdata/ folder.
Side note, technically speaking it's pretty ridiculous how Essentials still does a lot of main thread file writes. Maybe one should think about revamping the whole userdata management?
I've had players with +250 homes, and the time it takes to write that file after every single teleport did cause noticable performance issues. I have since limited the max amount of homes, but still, why do it on the main thread in the first place, and why write after every teleport just to update the last location? Why not read once on join on main thread, run deamon that saves every online player every x minute to disk and on logout, write on off thread.
Yeah, /baltop is not an issue, it would load tons of users into cache but I don't think that's the reason for memory leaks.
So far I've been using:
@Override
public void onEnable() {
....
getServer().getScheduler().scheduleSyncRepeatingTask(this, new Runnable()
{
public void run()
{
getUserMap().invalidateAll();
}
}, 20 * 60 * 60 * 1, 20 * 60 * 60 * 1);
....
}
I know this is a "dirty fix" but it would keep the size of userCache in somewhat sane numbers and prevent this:
@L4BORG On linux in the userdata folder how many files are NPCs and how many files do you have?
To check how many NPC files there are type cat * | grep -c "npc: true".
It might help to remove those same files that were purged from the usermap.csv.
Simply check if the line contains the uuid found in userdata. If it does, delete it and the line itself.
I did, I removed whole usermap.csv and let Ess regenerate the file, but the issue with memory is still there.
Probably because last location (for /back) is updated and with that written. I guess if you don't mind dataloss (OOM, crash, ...) writting that data on PlayerQuitEvent would be possible (or that + idk let say every 5 minutes or something).
But that's a whole new issue and it wouldn't fix memory leaks.
@SupaHam I did some purging (inactive players) and... NPC's are not the issue:
<?php
$count = 0;
$found = 0;
$dir = "/home/minecraft/survival/plugins/Essentials/userdata/";
$directoryIterator = new DirectoryIterator($dir);
foreach ($directoryIterator as $fileinfo)
{
if ($fileinfo->isDot())
continue;
$count++;
if($count%1000 == 0)
{
echo '.';
sleep(1);
}
$filename = $fileinfo->getFilename();
if(strpos(file_get_contents($dir.$filename), "npc: true") !== false)
$found++;
}
echo "\nTotal: ".$count."\nFound: ".$found;
user@host:~$ php tmp.php
..................................................................
Total: 66943
Found: 0
@L4BORG Is this now solved?