feat(feeds): add per-channel timeout to FEED_REFRESH loop#892
feat(feeds): add per-channel timeout to FEED_REFRESH loop#892jollySleeper wants to merge 1 commit into
Conversation
Wrap each channel refresh in a CompletableFuture with a 60-second timeout. ChannelInfo.getInfo() can make multiple HTTP calls per channel, so even though the downloader has a 10s per-request timeout, the aggregate time for one channel can exceed that significantly. A single hanging channel currently blocks the entire refresh loop indefinitely. With this change, if a channel refresh exceeds 60 seconds, it is skipped with a log message and the loop moves to the next channel.
📝 WalkthroughWalkthroughThe feed refresh loop now runs each channel update asynchronously with a 60-second wait, logs timed-out channels, unwraps execution failures to the exception handler, and stops the loop when interrupted. ChangesFeed refresh timeout handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/me/kavin/piped/Main.java`:
- Around line 301-303: The interruption handling in Main’s refresh loop only
breaks the inner for-loop, so the surrounding while(true) can continue after
Thread.currentThread().interrupt(). Update the InterruptedException catch in the
refresh thread logic to exit the entire refresh thread cleanly when interrupted,
using the refresh loop around the DB query/refresh pass as the target to stop
rather than only the inner loop.
- Around line 34-36: The feed refresh job in Main.refresh needs to be truly
cancellable instead of only timing out the wait on
CompletableFuture.runAsync(...).get(60, TimeUnit.SECONDS). Run the refresh work
on an interruptible task/thread, keep a handle to it, and explicitly cancel or
interrupt it when the timeout is hit so the work stops before mutating channel
state. Also update the InterruptedException handling in the refresh loop so it
exits the outer refresh flow, not just the inner channel loop, when an interrupt
is received.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3931f627-dc7d-4038-a176-8d0394dd63cc
📒 Files selected for processing (1)
src/main/java/me/kavin/piped/Main.java
| import java.util.concurrent.ExecutionException; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.TimeoutException; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🌐 Web query:
In Java 21, does CompletableFuture.get(long, TimeUnit) cancel or interrupt the asynchronous computation on TimeoutException, and does FutureTask.cancel(true) interrupt the runner?
💡 Result:
In Java 21, the behavior is as follows: 1. CompletableFuture.get(long, TimeUnit) and TimeoutException: Calling CompletableFuture.get(long, TimeUnit) does not cancel or interrupt the underlying asynchronous computation when a TimeoutException is thrown [1][2]. The TimeoutException only indicates that the calling thread has stopped waiting for the result; the task continues to execute in the background [1][2]. To stop the computation, you must explicitly call cancel(true) on the CompletableFuture instance after catching the TimeoutException [1][2]. Alternatively, you can use the non-blocking orTimeout(long, TimeUnit) method to cause the CompletableFuture to complete exceptionally with a TimeoutException if it does not finish within the specified time [3][4]. 2. FutureTask.cancel(true) and the Runner: Yes, FutureTask.cancel(true) attempts to interrupt the thread currently executing the task [5][6]. If the task has already started, passing true to the cancel method signals that the thread running the task should be interrupted [5][6]. The implementation sets the task state to INTERRUPTING and calls thread.interrupt on the runner [7][8]. However, the success of this interruption depends on whether the task code is responsive to interrupts (e.g., it checks the interrupted status or calls blocking methods that throw InterruptedException) [1][2][9].
Citations:
- 1: https://stackoverflow.com/questions/74640683/shouldnt-completablefuture-gettimeout-kill-the-task
- 2: https://stackoverflow.com/questions/49393194/set-a-timeout-on-individual-threads-when-using-completablefuture-using-a-threadp
- 3: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/concurrent/CompletableFuture.html
- 4: https://www.baeldung.com/java-completablefuture-timeout
- 5: https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/util/concurrent/FutureTask.html
- 6: https://docs.oracle.com/en/java/javase/23/docs/api/java.base/java/util/concurrent/FutureTask.html
- 7: https://android.googlesource.com/platform/libcore2/+/master/luni/src/main/java/java/util/concurrent/FutureTask.java
- 8: https://github.com/openjdk/jdk/blob/jdk8-b120/jdk/src/share/classes/java/util/concurrent/FutureTask.java
- 9: https://stackoverflow.com/questions/56680101/future-canceltrue-returns-true-but-tasks-not-cancelled
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of Main.java with line numbers
file="src/main/java/me/kavin/piped/Main.java"
wc -l "$file"
sed -n '260,320p' "$file" | cat -nRepository: TeamPiped/Piped-Backend
Length of output: 4114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the imports and the feed refresh block in Main.java
file="src/main/java/me/kavin/piped/Main.java"
sed -n '1,60p' "$file" | cat -n
printf '\n---\n'
sed -n '280,315p' "$file" | cat -nRepository: TeamPiped/Piped-Backend
Length of output: 5590
Make the feed refresh job cancellable.
CompletableFuture.runAsync(...).get(60, TimeUnit.SECONDS)only times out the waiter; the refresh keeps running on the common pool and can still mutate channel state after being skipped. Use an interruptible task/thread and cancel it on timeout.- The current
InterruptedExceptionhandler only breaks the inner channel loop, so an interrupt does not stop the outer refresh loop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/me/kavin/piped/Main.java` around lines 34 - 36, The feed
refresh job in Main.refresh needs to be truly cancellable instead of only timing
out the wait on CompletableFuture.runAsync(...).get(60, TimeUnit.SECONDS). Run
the refresh work on an interruptible task/thread, keep a handle to it, and
explicitly cancel or interrupt it when the timeout is hit so the work stops
before mutating channel state. Also update the InterruptedException handling in
the refresh loop so it exits the outer refresh flow, not just the inner channel
loop, when an interrupt is received.
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| break; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Exit the refresh thread on interruption.
Line 303 only breaks out of the for loop. Because the interrupt flag is restored, the surrounding while (true) can immediately start another DB query/refresh pass instead of shutting down cleanly.
Proposed fix
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
- break;
+ return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| break; | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/me/kavin/piped/Main.java` around lines 301 - 303, The
interruption handling in Main’s refresh loop only breaks the inner for-loop, so
the surrounding while(true) can continue after
Thread.currentThread().interrupt(). Update the InterruptedException catch in the
refresh thread logic to exit the entire refresh thread cleanly when interrupted,
using the refresh loop around the DB query/refresh pass as the target to stop
rather than only the inner loop.
|
QQ: Have you investigated why the |
2 participants
Summary
FEED_REFRESHloop with aCompletableFutureand a 60-second timeoutChannelInfo.getInfo()makes multiple HTTP calls per channel — even though the downloader has a 10s per-request timeout, the aggregate time for one channel can exceed that significantlyContext
PR #889 introduced the
FEED_REFRESHperiodic channel-refresh loop. The loop processes channels serially with paced delays, which is the right model for being a good citizen to YouTube's API. However, it has no per-channel time ceiling — if one channel's extraction hangs (network issues, YouTube serving unusual responses, etc.), the entire loop stalls.This adds a lightweight safety net: each channel gets up to 60 seconds to complete, which is generous given the 10s HTTP timeout, but prevents indefinite blocking.
Changes
Main.java: Wrap per-channel refresh inCompletableFuture.runAsync(...).get(60, TimeUnit.SECONDS)TimeoutException: log and skip to next channelExecutionException: unwrap cause and pass to existingExceptionHandlerInterruptedException: restore interrupt flag and exit loop cleanlyTest plan
./gradlew build)FEED_REFRESH=trueand verify normal refresh cycle completesSummary by CodeRabbit