ArcanePlugins / Treasury

🏦 A powerful multi-platform library for next-level plugin integrations.
https://hangar.papermc.io/ArcanePlugins/Treasury
Other
56 stars 13 forks source link

improve papi economy hook #210

Closed MrIvanPlays closed 1 year ago

MrIvanPlays commented 1 year ago

Attempt at fixing #208 from our side by replacing bad .join calls and making them balancecache and baltop work simultaneously. there are only 2 issues left: i don't like how im making the code busy wait and also it needs to be tested. ima attach a test jar in a minute or two after making this pr.

ive also added the future helper from #180 since i am pretty sure #180 will be soon closed and a new pr made to the dev/2.0.0 branch.

MrIvanPlays commented 1 year ago

treasury-plugin-bukkit.zip

MrIvanPlays commented 1 year ago

@Jikoo could you please review & test?

Jikoo commented 1 year ago

I wasn't able to fit it in this weekend, sorry. Will try to make time during the week

lokka30 commented 1 year ago

No worries @Jikoo!

MrIvanPlays commented 1 year ago

ok im not gonna provide a build this time. DIY

MrIvanPlays commented 1 year ago

cough

Jikoo commented 1 year ago

Looks like this just moved nullity handling problems around a little.

[09:34:01 WARN]: [Treasury] Plugin Treasury v1.2.1-0c619be-SNAPSHOT generated an exception while executing task 5
java.lang.NullPointerException: Cannot invoke "java.math.BigDecimal.compareTo(java.math.BigDecimal)" because "this.balance" is null
        at me.lokka30.treasury.plugin.bukkit.hooks.papi.economy.BalTop$TopPlayer.compareTo(BalTop.java:182) ~[treasury-bukkit-1.2.1-0c619be-SNAPSHOT.jar:?]
        at java.util.TreeMap.compare(TreeMap.java:1570) ~[?:?]
        at java.util.TreeMap.addEntryToEmptyMap(TreeMap.java:776) ~[?:?]
        at java.util.TreeMap.put(TreeMap.java:785) ~[?:?]
        at java.util.TreeMap.put(TreeMap.java:534) ~[?:?]
        at java.util.TreeSet.add(TreeSet.java:255) ~[?:?]
        at com.google.common.collect.AbstractMapBasedMultimap.put(AbstractMapBasedMultimap.java:191) ~[guava-31.0.1-jre.jar:?]
        at com.google.common.collect.AbstractSetMultimap.put(AbstractSetMultimap.java:140) ~[guava-31.0.1-jre.jar:?]
        at me.lokka30.treasury.plugin.bukkit.hooks.papi.economy.BalTop.run(BalTop.java:140) ~[treasury-bukkit-1.2.1-0c619be-SNAPSHOT.jar:?]
        at org.bukkit.craftbukkit.v1_19_R1.scheduler.CraftTask.run(CraftTask.java:101) ~[paper-1.19.2.jar:git-Paper-245]
        at org.bukkit.craftbukkit.v1_19_R1.scheduler.CraftAsyncTask.run(CraftAsyncTask.java:57) ~[paper-1.19.2.jar:git-Paper-245]
        at com.destroystokyo.paper.ServerSchedulerReportingWrapper.run(ServerSchedulerReportingWrapper.java:22) ~[paper-1.19.2.jar:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
        at java.lang.Thread.run(Thread.java:833) ~[?:?]
[09:34:31 WARN]: [Treasury] Couldn't update baltop placeholders for PlaceholderAPI!

Would also recommend that we make the latch an AtomicReference<CountDownLatch> instead.

/e: Was going to add a commit for nullity handling and offer one for the AtomicReference suggestion but apparently my GPG key has expired. Looks like this will take a bit more time than expected.

Jikoo commented 1 year ago

I've also pushed 55af1220ddcaa5519c7f5a33ecff834566073649 to my fork as a suggestion for eliminating the (incredibly slight) risk of issues when accessing the latch concurrently with reassignment. Can push that in here if we approve.

lokka30 commented 1 year ago

Thanks Jikoo. :)

MrIvanPlays commented 1 year ago

@Jikoo your change to AtomicReference is tested, right?

MrIvanPlays commented 1 year ago

baltop is broken, working on a fix rn

Jikoo commented 1 year ago

I thought it was functioning when I tested, but I could well have been using a bad build. Thanks for the fix!