Trophonix / TradePlus

Easy-to-use, highly configurable trading plugin for Spigot- and Bukkit-based Minecraft servers.
https://www.spigotmc.org/resources/23138/
GNU General Public License v3.0
54 stars 42 forks source link

Fix stackoverflow by converting TradeLogs to Strings on write #63

Closed leviem1 closed 3 years ago

leviem1 commented 3 years ago

This is totally untested as I wasn't able to pull the PluginBase dependency easily (#57), but should fix Trophonix/TradePlus#60 and fix Trophonix/TradePlus#62 if I didn't mess anything up.

I believe the root of the stackoverflow has something to do with ItemStack not being serializable (org.bukkit.configuration.serialization.ConfigurationSerializable != java.io.Serializable), so I've switched to using Strings.

I'm making an assumption in this patch that there shouldn't be a reason we would create a TradeLog from the string that is written to the file since that seems like overkill for basic logging. If that's a bad assumption, we should implement a better TypeAdapter that will handle the serialization of ItemStacks.

This small fix took me way longer than it should have, so if it works you owe me a coffee! :laughing:

Always happy to help!

Trophonix commented 3 years ago

Thank you so much for your effort 😄 I am loading it up to test it, I think I'm going to want to implement ItemStack serialization, though. I think this will look like {DIAMOND_SWORD x 1}, {POTATO x 50} and some players use it to trade specific special items so the admin would need to see the display name or enchantments to know the information they need. 😃 I didn't realize this was the problem causing the stack overflows.

The lag at the beginning of a trade might is a separate issue. When I do my first trade on my server, it says loading legacy material support even on 1.16, which I thought I had avoided with my versioning but it still does it for some reason. Maybe because of the api-version in my plugin.yml? Or it could just be the way I am instantiating ItemStacks, I need to figure that one out.

leviem1 commented 3 years ago

Yeah, serialization seems like the more "correct" way of doing it, though then you have to worry about the deserialization which was more than I wanted to do with a quick hack 😆

Feel free to either commit to this branch with any changes you'd like to make or close it. I could definitely look into the serialization too, but it could take a me a bit.

I happen to not get the material support issue since I'm running essentials or something that loads that already at server start.

leviem1 commented 3 years ago

Also, a few possible culprits

Trade.java Screen Shot 2020-08-19 at 5 40 35 AM

MsgUtils1_8.java Screen Shot 2020-08-19 at 5 41 15 AM

leviem1 commented 3 years ago

Thinking about it weeks later... you could probably make a wrapper object for the item stack that is serializable.