Potential crashes with depositing money to offline players

[OmerBenGera]

Information

Full output of /ess version: Every version of Essentials probably.

Details

Description
When using depositPlayer on OfflinePlayer objects, it has a potential to crash if used often. When using the depositPlayer method from Vault, Essentials tries to get the user from an offline player. But, in the get method - it uses OfflinePlayer#setName method, which loads data from the player's file. This task, can cause issues and crash the server.

Steps to reproduce
It was reported by a customer of mine, I can only give the crash reports which shows exactly what the issue is: https://pastebin.com/w1Gzp6Ui

• WildChests's sell chest is filled with an item
• WildChests deposits money to the player
• Essentials calls setName
• Server crashes

[pop4959]

Several problems with this issue:

1) You haven't mentioned what version of Essentials you are using, nor any details about the server. 2) There are many stack traces being printed out in that log, Essentials only being contained in one of them. Watchdog prints out stack traces when the server is lagging badly in general, and what is contained in them depends on what's being processed at the time it prints (which may be arbitrary). Since not all 3 stack traces are the same plugin, it's not as likely that the lag is related specifically to any one plugin, and especially not Essentials. 3) Your log doesn't show anything of what occurred before the stack trace with Essentials. 4) The stack trace containing Essentials does not "crash" on setName. The current method being called was java.util.zip.Inflater.inflateBytes(Native Method). The plugin triggering this was also com.bgsoftware.wildchests.nms.NMSInventory_v1_15_R1$TileEntityWildChest.setItem.

As a result, no matter how much we may like to, we cannot look further into this issue as is. This could just as easily be "normal" server lag, as far as I can tell. If you have reason to believe that this is still an issue with Essentials, please do the following:

1) Fill out the issue template entirely, with full and correct information. 2) Replicate the issue using a method that can be triggered within Essentials, not using other plugins (which may be causing lag themselves). 3) Use some performance monitoring tools like Aikar's timings, or Luck's spark, to show that Essentials methods are causing lag.

Feel free to make a new issue if you have done the above and have found a way to consistently replicate the problem and can accurately track it to Essentials.

[OmerBenGera]

1. Happens with latest and every version that uses OfflinePlayer#setName, as stated in the report.
2. The length of the stack trace really means nothing. As stated in the report, WildChests calls the depositPlayer, and from the stack trace you can clearly see that setName opens the player data, and changes data there - and sometimes, because of too many calls to that method, it crashses the server. Don't blame other plugins, it's the OfflinePlayer#setName method that causes it.
3. Are you kiddnig me? lol. I literally gave you the entire stack trace, and it's clearly states there what happened. But let me explain you, again:
   • An item is added to a WildChest's sell chest.
   • WildChests runs Economy#depositPlayer (line 47)
   • Essentials get the offline user instance (line 39)
   • Essentials calls the setName method of the OfflinePlayer instance that was provided (line 38)
   • The minecraft game code opens the player data from the world (line 34)
   • The game crashes because it's a heavy task that shouldn't be called. If you just had opened the stack trace and put a bit of an effort to read it, then you would understand everything.
4. That's so false and not correct. All of that mess (opening files etc) is done from the setName method of OfflinePlayer. denying it is just denying the issue. Here's a screenshot that can confirm that OfflinePlayer#setName is opening a file (by calling the Bukkit OfflinePlayer getName method): https://gyazo.com/3b32254b728e3f6780b0547643423526 In line 58, the getBukkitData method is called, which is opening player's file data (can be seen by code / stack trace)

I'm looking into reopening the issue as it's a serious one that should be fixed.

[pop4959]

What's Essentials supposed to do about this though? Do you expect us to buy a premium plugin just to test this? Doesn't seem reasonable. Most likely, if this is an issue, it's a result of poor use of the method by the plugin. Offline player methods are not generally known to be efficient. I don't think this is really Essentials' fault (yes it is calling the method, but the plugin should be accounting for this and not making assumptions about its speed). Obviously calling a slow method over and over in the same tick is going to cause watchdog to kill the server, and it's being called by the other plugin.

Once again, I'm simply asking you to point out an instance where this crash can be caused by the method in Essentials itself. It should be fairly easy to cause, given how commonly getUser is called, however there are no reports of this being a problem. If it can't be caused by Essentials, it's very clear that the other plugin is doing something wrong. Perhaps not checking if the user is offline, not efficiently calling the method, or calling the method much too frequently.

I will personally re-open this issue for you if you have enough information to prove that this is a problem with Essentials. Your response still isn't substantive enough, having only given information about the CraftOfflinePlayer::getName method call being relatively slow due to opening a file, which is not really a bug.

[OmerBenGera]

You can fix it by just not calling it? That's a simple fix that can be done by changing one line of the plugin... I like how denial you are to the issues with the plugin, and then when a customer reports the issue (not me, the actual customer that reported it to you), one of your developers told him that "he hates when people telling how Essentials is bad, instead of reporting the issues". Not only that no one said that Essentials is bad, but look how you handle the issues that are reported - just closing them without checking that out. You clearly admitted that getName is not performance friendly, but you still don't want to get rid of it? I don't understand then how it has to do with any other plugin, and why can't you just fix it..?

[JRoy]

No need to get hostile. Without your code I can't really see what's going on within your plugin as to what's the real issue here. It's not as simple as "just don't call it". Essentials internals are scary and removing that line would likely break everything. I understand you have a premium plugin so if we could continue this privately on discord so you can provide me the relative source code that's causing this issue. My discord is Joshie#0001