search

elBukkit / PreciousStones

Self-service protection for Minecraft servers
http://dev.bukkit.org/server-mods/preciousstones/
10 stars 10 forks source link

Fix Timeout Issues w/ Clans #115

Closed FireController1847 closed 3 years ago

FireController1847 commented 3 years ago

What was happening was that, if the clan was not found, it would continue forward to check if it is a player. But, the problem is, if we are trying to allow a clan, the player will simply never exist. So it always takes the maximum amount of lookup time to attempt to find a player offline that never exists, not to mention the excessive amount of API requests being made to the Mojang API to attempt to find these players that don't exist.

My PR short-circuits the logic to return false if a clan is not found. From what I can tell, it works the same as before but doesn't lock up the server and speeds up the method for clans tremendously.

Fixes #114

xtomyserrax commented 3 years ago

Thank you so much for taking your time. Have you tested it? Im going to try it in my test server and if everything works fine Ill merge.

There are some methods too that could cause timeout issues. Sadly proper imporvements would mean a recode. ACF would be a beautiful addition to this plugin but it also means an insane amount of work haha.

FireController1847 commented 3 years ago

I have tested it on a private test server and it appeared to work, however I do have a report on my live server that it did not work. I wasn't there when they tested that so I don't know the validity of it, I am going to do some proper testing with my friend here shortly. So far I haven't noticed any issues but I'll report back here on that second test

I totally understand that. I may take up writing a protection plugin from the ground up that has a similar concept to PreciousStones, where the protection is based on blocks, but that would be a total side project I'm not sure if I want at the moment haha

xtomyserrax commented 3 years ago

If you could test it in your live server it would be amazing. In my case I would also test it in my test one which may not be the best scenario!

FireController1847 commented 3 years ago

Unfortunately I can confirm this did not fix the issue. I will do some more investigation soon.

FireController1847 commented 3 years ago

This goes a lot deeper than I would have thought. For each field it's taking ~80ms which is unacceptable, and every once in a while you get fields that take between ~1000 and ~5000ms. I'm going to have to rewrite quite a bit of this logic to alleviate this issue. I'll work on that for most of today, hopefully I can get it done by tonight if not tomorrow.

FireController1847 commented 3 years ago

I rewrote a good chunk of the logic and I have completely alleviated the issue, appears to work. Will run a production test with one of my members tomorrow :)

FireController1847 commented 3 years ago

I move at lightning speed ⚡

I ran a test on my personal fields on the prod server, ran /ps allowall c:faketest. Allowed it to 583 fields, worked really, really quickly and didn't have an issue. Allowed to all of them as it was supposed to. Seems to be working

FireController1847 commented 3 years ago

Update as of tomorrow. I don't know if this PR is causing this, but when it has been allowed it no longer works... I don't.. arrrgggggggggg I will figure this out bare with me. Do not merge

xtomyserrax commented 3 years ago

Thank you for all your work. Yeah, dont feel rushed, make a good testing please and after that Ill test it myself. Im going on holidays on friday so I might not answer for a week. Thanks!

FireController1847 commented 3 years ago

Yeah, alrighty, I'll keep it hosted on my production server and fix issues as they arise

FireController1847 commented 3 years ago

Alright, it's been a while and so far we've had no additional issues as with my previous fix. Should be ready for your testing and hopefully, cross my fingers, to go into production! 🙂

Also, sorry for my previous commit message, it was late and I was frustrated that I had to apply a fix yet again (and my subsequent failure of claiming I had fixed the issue - three times later).

Through the last two fixes I modified the code to not even touch groups - so this PR only fixes issues with clans. I don't know if there are the same issues with groups, but I also don't know how groups works...

xtomyserrax commented 3 years ago

Hey, thank you so much! If you have the jar please send it to me as Im lazy to generate it haha. If not Ill do it.

I just got back from holidays, will play a little bit with it, check the code, and if everything works fine lets go!

Thanks for taking your time!

FireController1847 commented 3 years ago

PreciousStones-1.16.1.12-pr115.zip

xGlenor commented 3 years ago

Is this logic only fixed under SimpleClans? My server was unresponsive and even crashes when added to the normal plot when there were a lot of them

xtomyserrax commented 3 years ago

Is this logic only fixed under SimpleClans? My server was unresponsive and even crashes when added to the normal plot when there were a lot of them

Did it still crash before this update @xGlenor ?

Did you have any other issue @FireController1847 ?

xGlenor commented 3 years ago

I need time to test. If I find out, I'll let you know immediately. As I wrote, we are in the process of rebuilding the servers after the OVH fire, so testing may take quite a long time

FireController1847 commented 3 years ago

Did you have any other issue @FireController1847 ?

No, I've not had any other issues so far and my server has been in production using this PR for a few weeks now.

xtomyserrax commented 3 years ago

Ill test as soon as Im able to (sorry but Im really busy with work). If you @xGlenor can let us know about that it will help.

And Im sorry to hear about that OVH thing.

Thanks to both of you for your time

xtomyserrax commented 3 years ago

Heeey, were you able to test this? @xGlenor

How has this been doing @FireController1847 ?

Thanks!

FireController1847 commented 3 years ago

How has this been doing @FireController1847

We've had no issues on our production server with this PR and we've been running it for a while

xGlenor commented 3 years ago

Hello :D I turned on the command for all players. Everything works initially. Sorry to write so late. I finished school and I am writing exams. I have one more left :D

xtomyserrax commented 3 years ago

Thanks so much for the help!

xGlenor commented 3 years ago

Hi :D I have updated the plugin on my test server because I am creating a GUI plugin. After adding people to the field, I still noticed the lag issue I also included a video: https://www.youtube.com/watch?v=IXNG7GkAaEo There were only 25 fields on the server, only one player online

FireController1847 commented 3 years ago

@xGlenor Sorry about that. This PR actually only fixes issues with clans specifically, not players. The issue here is that the plugin attempts to fetch the offline player for every field (instead of caching it once beforehand and checking then). Since getOfflinePlayer will make a request to the Mojang API if it's not immediately found in server cache, it causes a request each field and eventually causes the slowdown you noticed. This should be fixed, I agree, but was out of scope for this PR.

xGlenor commented 3 years ago

I understand, sorry for the mistake.

FireController1847 commented 3 years ago

I understand, sorry for the mistake.

You're good! I think it highlights an important concern that this PR doesn't fix and shows how this command is still flawed.

xtomyserrax commented 3 years ago

This lag could also be related with your database. Doesnt it lag with other plugins?

I know some big servers that use PS and it works fine for them. Though this command does have some issues and could potentially crash servers like told in #108

Fixing this would require some reworks, time and will haha

FireController1847 commented 3 years ago

I can tell you exactly what line is causing the issue, and I suppose I could take a shot at fixing it. It's the line I added the TODO in this PR to, it will lag only a little from an existing player because it can find their data easily and (as shown in the video) the check works wonderfully when using an existing player. However, if a player does not exist, because of getOfflinePlayer it will take forever to make the request since it's being done for each field.

The only reason this PR works is because it short-circuits the player check when using a clan name. And you don't see any other clan issues when it does use the player check since every time that is called the player is guaranteed to exist.

You're right, a refactor would be needed to fix this issue. However I do think there are a few options, likely the best one being to either add a utility method for getPlayer that only runs getOfflinePlayer if we know the player exists (as in, they've logged in once), or the second being to eliminate getOfflinePlayer all together. Third one is to alternatively continue using getOfflinePlayer but cache the player search before even entering the loop, keeping the player only once. This option would require the most refactoring but would yield the best outcome.

Honestly, I just wish Spigot would add an option to the method as to whether or not you want to have it make that request to Mojang to get the data or not. I don't see why they would require that, but hey, that's how it is.

I'll see what I can do the next time I get a chance (maybe within the next few days).