Fix: NoSuchElementException in ChumBucketHider (#2587)

Co-authored-by: Linnea Gräf <nea@nea.moe>

3 files changed, 15 insertions, 10 deletions

diff --git a/src/main/java/at/hannibal2/skyhanni/features/fishing/ChumBucketHider.kt b/src/main/java/at/hannibal2/skyhanni/features/fishing/ChumBucketHider.kt
index 0bb98f8e4..5f64aba8c 100644
--- a/src/main/java/at/hannibal2/skyhanni/features/fishing/ChumBucketHider.kt
+++ b/src/main/java/at/hannibal2/skyhanni/features/fishing/ChumBucketHider.kt
@@ -66,15 +66,7 @@ object ChumBucketHider {
         // Chum Bucket
         if (config.hideBucket.get() && entity.inventory.any { it != null && (it.name == "§fEmpty Chum Bucket" || it.name == "§aEmpty Chumcap Bucket") }) {
             val entityLocation = entity.getLorenzVec()
-            val toSet = try {
-                titleEntity.toSet()
-            } catch (e: NoSuchElementException) {
-                // Caught an NoSuchElementException in ChumBucketHider at CheckRenderEntityEvent: null
-                // We know this happens, but we cant fix it, apparently.
-                // TODO fix it anyway
-                return
-            }
-            for (title in toSet) {
+            for (title in titleEntity.toSet()) {
                 if (entityLocation.equalsIgnoreY(title.getLorenzVec())) {
                     hiddenEntities.add(entity)
                     event.cancel()
diff --git a/src/main/java/at/hannibal2/skyhanni/utils/TimeLimitedCache.kt b/src/main/java/at/hannibal2/skyhanni/utils/TimeLimitedCache.kt
index 4c96c9b3c..a88db58e1 100644
--- a/src/main/java/at/hannibal2/skyhanni/utils/TimeLimitedCache.kt
+++ b/src/main/java/at/hannibal2/skyhanni/utils/TimeLimitedCache.kt
@@ -48,11 +48,24 @@ class TimeLimitedCache<K : Any, V : Any>(
      * Modifications to the returned map are not supported and may lead to unexpected behavior.
      * This method is intended for read-only operations such as iteration or retrieval of values.
      *
+     * This returning map and any view into that map via [Map.keys], [Map.values] or [Map.entries],
+     * may return [Collection.size] values larger than the elements actually present during iteration.
+     * This can lead to problems with kotlins [Iterable.toSet], [Iterable.toList] (etc.) small collection
+     * optimizations. Those methods (and similar ones) have optimizations for single element collections.
+     * Since the [Collection.size] is checked first those methods will then not make any additional
+     * checks when accessing the elements of the collection. This can lead to rare [NoSuchElementException].
+     * Therefore, the direct constructors of [HashSet], [ArrayList] and similar are to be preferred,
+     * since they make no such optimizations.
+     *
      * @return A read-only view of the cache's underlying map.
      */
     private fun getMap(): ConcurrentMap<K, V> {
         val asMap: ConcurrentMap<K, V>
 
+        // TODO: the returned map view here is live and is updated as the underlying cache is.
+        //       the lock here is relatively pointless. i don't know why it was put here, but i
+        //       can't imagine a good reason, since every access to the returned map would also
+        //       need to be guarded by the same lock to be effective.
         cacheLock.readLock().lock()
         try {
             asMap = cache.asMap()
diff --git a/src/main/java/at/hannibal2/skyhanni/utils/TimeLimitedSet.kt b/src/main/java/at/hannibal2/skyhanni/utils/TimeLimitedSet.kt
index 100bbbd10..710e7ee46 100644
--- a/src/main/java/at/hannibal2/skyhanni/utils/TimeLimitedSet.kt
+++ b/src/main/java/at/hannibal2/skyhanni/utils/TimeLimitedSet.kt
@@ -27,7 +27,7 @@ class TimeLimitedSet<T : Any>(
 
     fun clear() = cache.clear()
 
-    fun toSet(): Set<T> = cache.keys().toSet()
+    fun toSet(): Set<T> = HashSet(cache.keys())
 
     override fun iterator(): Iterator<T> = toSet().iterator()
 }