From 0d281d483909d71272783033b2aba8f33dcbce36 Mon Sep 17 00:00:00 2001 From: Roman / Linnea Gräf Date: Sat, 18 Feb 2023 15:24:52 +0100 Subject: ApiCache: Fix apparent NullPointerException (#619) `CacheResult` should in theory have either a `file != null` a `future != null` or be `disposed`. Apparently this invariant of `CacheResult` is either being violated somewhere, or the `synchronized` blocks arent as synchronized as id hoped they were. In fact, `dispose()` does not even delete the file, so i can really only see this happening because the first `synchronized` block that writes the file and the second `synchronized` block that reads from the file hold the same lock. I have no idea how this would happen, but hopefully this fixes it (since the dispose didn't have a threading issue reported so far, i feel more confident leaving the .deleteOnExit in there, but I'm also wrapping any potential IOExceptions during access, because I am just so confused how the internal state was broken. --- .../moulberry/notenoughupdates/util/ApiCache.kt | 52 +++++++++++----------- .../util/kotlin/completablefuture.kt | 33 ++++++++++++++ 2 files changed, 60 insertions(+), 25 deletions(-) create mode 100644 src/main/kotlin/io/github/moulberry/notenoughupdates/util/kotlin/completablefuture.kt (limited to 'src/main/kotlin/io') diff --git a/src/main/kotlin/io/github/moulberry/notenoughupdates/util/ApiCache.kt b/src/main/kotlin/io/github/moulberry/notenoughupdates/util/ApiCache.kt index c14df425..59fc2dd5 100644 --- a/src/main/kotlin/io/github/moulberry/notenoughupdates/util/ApiCache.kt +++ b/src/main/kotlin/io/github/moulberry/notenoughupdates/util/ApiCache.kt @@ -22,6 +22,7 @@ package io.github.moulberry.notenoughupdates.util import io.github.moulberry.notenoughupdates.NotEnoughUpdates import io.github.moulberry.notenoughupdates.options.customtypes.NEUDebugFlag import io.github.moulberry.notenoughupdates.util.ApiUtil.Request +import io.github.moulberry.notenoughupdates.util.kotlin.supplyImmediate import org.apache.http.NameValuePair import java.nio.file.Files import java.nio.file.Path @@ -47,43 +48,45 @@ object ApiCache { val shouldGunzip: Boolean, ) - data class CacheResult( - private var future: CompletableFuture?, + data class CacheResult internal constructor( + var cacheState: CacheState, val firedAt: TimeSource.Monotonic.ValueTimeMark, - private var file: Path? = null, - private var disposed: Boolean = false, ) { - init { - future!!.thenAcceptAsync { text -> + constructor(future: CompletableFuture, firedAt: TimeSource.Monotonic.ValueTimeMark) : this( + CacheState.WaitingForFuture(future), + firedAt + ) { + future.thenAccept { text -> synchronized(this) { - if (disposed) { - return@synchronized - } - future = null val f = Files.createTempFile(cacheBaseDir, "api-cache", ".bin") log("Writing cache to disk: $f") f.toFile().deleteOnExit() f.writeText(text) - file = f + cacheState = CacheState.FileCached(f) } } } - val isAvailable get() = file != null && !disposed + sealed interface CacheState { + object Disposed : CacheState + data class WaitingForFuture(val future: CompletableFuture) : CacheState + data class FileCached(val file: Path) : CacheState + } + + val isAvailable get() = cacheState is CacheState.FileCached fun getCachedFuture(): CompletableFuture { synchronized(this) { - if (disposed) { - return CompletableFuture.supplyAsync { - throw IllegalStateException("Attempting to read from a disposed future at $file. Most likely caused by non synchronized access to ApiCache.cachedRequests") + return when (val cs = cacheState) { + CacheState.Disposed -> supplyImmediate { + throw IllegalStateException("Attempting to read from a disposed future. Most likely caused by non synchronized access to ApiCache.cachedRequests") } - } - val fut = future - if (fut != null) { - return fut - } else { - val text = file!!.readText() - return CompletableFuture.completedFuture(text) + + is CacheState.FileCached -> supplyImmediate { + cs.file.readText() + } + + is CacheState.WaitingForFuture -> cs.future } } } @@ -96,11 +99,10 @@ object ApiCache { */ internal fun dispose() { synchronized(this) { - if (disposed) return + val file = (cacheState as? CacheState.FileCached)?.file log("Disposing cache for $file") - disposed = true + cacheState = CacheState.Disposed file?.deleteIfExists() - future = null } } } diff --git a/src/main/kotlin/io/github/moulberry/notenoughupdates/util/kotlin/completablefuture.kt b/src/main/kotlin/io/github/moulberry/notenoughupdates/util/kotlin/completablefuture.kt new file mode 100644 index 00000000..de45c1e3 --- /dev/null +++ b/src/main/kotlin/io/github/moulberry/notenoughupdates/util/kotlin/completablefuture.kt @@ -0,0 +1,33 @@ +/* + * Copyright (C) 2023 NotEnoughUpdates contributors + * + * This file is part of NotEnoughUpdates. + * + * NotEnoughUpdates is free software: you can redistribute it + * and/or modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation, either + * version 3 of the License, or (at your option) any later version. + * + * NotEnoughUpdates is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with NotEnoughUpdates. If not, see . + */ + +package io.github.moulberry.notenoughupdates.util.kotlin + +import java.util.concurrent.CompletableFuture + +inline fun supplyImmediate(block: () -> R): CompletableFuture { + val cf = CompletableFuture() + try { + cf.complete(block()) + } catch (t: Throwable) { + cf.completeExceptionally(t) + } + return cf +} + -- cgit