jpenilla / AnnouncerPlus

Announcement and Join/Quit message plugin with support for permissions. Supports Hex colors, gradients, Achievements, Titles, Action Bars, and more. Uses MiniMessage for text formatting
https://www.spigotmc.org/resources/announcer-plus.81005/
MIT License
70 stars 13 forks source link

[Bug Report] Recursive scheduling of asynchronous tasks in case of missing messages; #224

Closed HarvelsX closed 3 months ago

HarvelsX commented 3 months ago

Describe the bug If no messages are available on the broadcast, the plugin cyclically schedules to process a message queue with no messages in it, which causes constant scheduling.

To Reproduce

  1. Set -Xms=-Xmx=1G
  2. Remove all messages from demo.conf (messages=[])
  3. Wait a while (1 day?)
  4. See the memory leak

Expected behavior Ignoring scheduling of tasks without messages, or optimizing scheduling

Screenshots image image

Server Software and Version:

Suggestions for solutions Remove recursive scheduling

diff --git a/src/main/kotlin/xyz/jpenilla/announcerplus/config/message/MessageConfig.kt b/src/main/kotlin/xyz/jpenilla/announcerplus/config/message/MessageConfig.kt
index 699be5b..d4a8fe9 100644
--- a/src/main/kotlin/xyz/jpenilla/announcerplus/config/message/MessageConfig.kt
+++ b/src/main/kotlin/xyz/jpenilla/announcerplus/config/message/MessageConfig.kt
@@ -204,15 +204,13 @@ class MessageConfig : SelfSavable<CommentedConfigurationNode>, KoinComponent {
   @Transient
   private val broadcastQueue = ArrayDeque<Message>()

-  fun broadcast(skipInitialDelay: Boolean = false) {
-    stop()
-    broadcastQueue.clear()
-    broadcastQueue.addAll(shuffledMessages())
-    broadcastTask = announcerPlus.asyncTimer(if (skipInitialDelay) 0L else initialDelay.ticks, interval.ticks) {
+  fun broadcast() {
+    broadcastTask = announcerPlus.asyncTimer(initialDelay.ticks, interval.ticks) {
       if (broadcastQueue.isNotEmpty()) {
         broadcast(broadcastQueue.removeFirst())
       } else {
-        broadcast(true)
+        // Removed recursion to prevent out of memory errors with empty messages
+        broadcastQueue.addAll(shuffledMessages())
       }
     }
   }
jpenilla commented 3 months ago

Old tasks were already being cancelled, I've removed the redundant scheduling when there is no messages however the real impact is probably next to zero.

HarvelsX commented 3 months ago

@jpenilla, every clock cycle a new task was scheduled, even if the previous one was canceled, the scheduling costs could play a role. If necessary, I can provide a memory dump with many tasks of this lambda.

jpenilla commented 3 months ago

If the file is not too large I guess I can take a look, but I do not see any way for a meaningful memory leak (or any memory leak at all) to happen here, the tasks are not capturing the previous task or anything like that.

HarvelsX commented 3 months ago

@jpenilla, here are some screenshots from Eclipse MAT and the dump itself image image

hprof: https://drive.google.com/file/d/1RMt15DhFI2G-1e9aZ5QXhq1kCJlNHQT8/view

jpenilla commented 3 months ago

Looks like some sort of bug with the async scheduler or another plugin

f201bb1ef190f0c61385dcaca560f4aa71bb53a1 this will avoid the issue

HarvelsX commented 3 months ago

another plugin

It's also a possibility, I'll continue to look into issue when similar problems arise.

Thanks a lot for the very fast support, you are the best!