v0.4.21: extract publishIfNotCancelled, close main download publish race
Some checks failed
build / build (push) Has been cancelled
Some checks failed
build / build (push) Has been cancelled
Reviewer-flagged: v0.4.20 fixed the reindex branch's post-put race, but the main download completion path still did READY.put without a follow-up epoch check. /videoCache clear landing between READY.put and the chat notification could leave a resurrected READY entry pointing at a deleted file, plus a stale "완료" message. Extracted publishIfNotCancelled(url, path, startEpoch) helper that does the full pre-check → put → post-check → rollback dance, and routed both publish sites (reindex branch + download-complete branch) through it. Centralizing this is the structural fix: future publish sites can't forget the post-check.
This commit is contained in:
@@ -5,7 +5,7 @@ org.gradle.configuration-cache=false
|
||||
|
||||
# Mod
|
||||
mod_id=video_player
|
||||
mod_version=0.4.20
|
||||
mod_version=0.4.21
|
||||
maven_group=com.ejclaw.videoplayer
|
||||
archives_base_name=video_player
|
||||
|
||||
|
||||
@@ -233,6 +233,33 @@ public final class VideoCache {
|
||||
|
||||
// -- internals -----------------------------------------------------------------------
|
||||
|
||||
/**
|
||||
* Publish {@code path} into {@link #READY} for {@code url}, but only if the cache
|
||||
* epoch hasn't moved since {@code startEpoch} was captured. Wraps the
|
||||
* pre-check / put / post-check / rollback dance so every publish site uses the
|
||||
* same race-free pattern instead of re-implementing it.
|
||||
*
|
||||
* <p>Race coverage: if {@link #clearAll()} runs at any point between the
|
||||
* pre-check and the post-check, the post-check sees the epoch bump and we
|
||||
* roll back — {@link Map#remove(Object, Object)} is a compareAndRemove so we
|
||||
* don't clobber a legitimate concurrent put under the same key, and we delete
|
||||
* the on-disk file because clearAll's directory scan may have missed it (the
|
||||
* file may not have existed yet when clearAll ran).
|
||||
*
|
||||
* @return {@code true} if the entry is now published; {@code false} if the
|
||||
* caller's work was cancelled and {@code path} has been cleaned up.
|
||||
*/
|
||||
private static boolean publishIfNotCancelled(String url, Path path, long startEpoch) {
|
||||
if (CACHE_EPOCH.get() != startEpoch) return false;
|
||||
READY.put(url, path);
|
||||
if (CACHE_EPOCH.get() != startEpoch) {
|
||||
READY.remove(url, path);
|
||||
try { Files.deleteIfExists(path); } catch (Throwable ignored) {}
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
private static void download(String url, long startEpoch) {
|
||||
Path cacheDir = cacheDir();
|
||||
Path partPath = null;
|
||||
@@ -259,18 +286,7 @@ public final class VideoCache {
|
||||
// Resume-friendly: if the file's already on disk from an earlier session, just
|
||||
// index it without re-downloading.
|
||||
if (Files.exists(finalPath) && Files.size(finalPath) > 0) {
|
||||
// Pre-publish check — bail if clearAll has run since submit.
|
||||
if (CACHE_EPOCH.get() != startEpoch) return;
|
||||
READY.put(url, finalPath);
|
||||
// Post-publish re-check: same window as the post-move check on the download
|
||||
// path. If clearAll landed between the epoch read above and the READY.put,
|
||||
// it would have wiped READY + the file, and our put() resurrected a stale
|
||||
// entry pointing at a now-deleted path. Roll it back: remove our entry
|
||||
// (compareAndRemove via remove(key, value) to avoid clobbering a concurrent
|
||||
// legitimate re-put) and best-effort delete the file.
|
||||
if (CACHE_EPOCH.get() != startEpoch) {
|
||||
READY.remove(url, finalPath);
|
||||
try { Files.deleteIfExists(finalPath); } catch (Throwable ignored) {}
|
||||
if (!publishIfNotCancelled(url, finalPath, startEpoch)) {
|
||||
VideoPlayerMod.LOG.info("[{}] preload: reindex cancelled (clearAll ran) — {}",
|
||||
VideoPlayerMod.MOD_ID, url);
|
||||
return;
|
||||
@@ -370,21 +386,16 @@ public final class VideoCache {
|
||||
Files.move(partPath, finalPath, StandardCopyOption.REPLACE_EXISTING,
|
||||
StandardCopyOption.ATOMIC_MOVE);
|
||||
|
||||
// Post-move cancellation check. clearAll() may have run between the pre-move
|
||||
// check above and the move itself — in that case clearAll's directory scan
|
||||
// either missed our file (it didn't exist yet) or saw the .part and skipped /
|
||||
// failed to delete it. Either way, finalPath now exists on disk but the
|
||||
// user-visible cache state is "cleared", so we must delete the file and skip
|
||||
// both the READY.put and the "완료" chat. Without this check, a clear right at
|
||||
// this window leaves a resurrected file in READY and a stale "완료" message.
|
||||
if (CACHE_EPOCH.get() != startEpoch) {
|
||||
VideoPlayerMod.LOG.info("[{}] preload: cancelled after move (clearAll ran) — {}",
|
||||
// Race-safe publish: this covers the window between Files.move and READY.put
|
||||
// (clearAll deletes the freshly-moved file then we resurrect it in READY) AND
|
||||
// the window between READY.put and the "완료" notification (clearAll wipes
|
||||
// READY then we re-publish + emit stale completion chat). publishIfNotCancelled
|
||||
// rolls back both READY and the on-disk file if the epoch moved at any time.
|
||||
if (!publishIfNotCancelled(url, finalPath, startEpoch)) {
|
||||
VideoPlayerMod.LOG.info("[{}] preload: cancelled at publish (clearAll ran) — {}",
|
||||
VideoPlayerMod.MOD_ID, url);
|
||||
try { Files.deleteIfExists(finalPath); } catch (Throwable ignored) {}
|
||||
return;
|
||||
}
|
||||
|
||||
READY.put(url, finalPath);
|
||||
VideoPlayerMod.LOG.info("[{}] preload: cached {} ({} bytes) -> {}",
|
||||
VideoPlayerMod.MOD_ID, url, total, finalPath.getFileName());
|
||||
long mb = Math.max(1, total / (1024 * 1024));
|
||||
|
||||
Reference in New Issue
Block a user