Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use multiple threads for refreshing feeds #7126

Merged
merged 6 commits into from Apr 27, 2024
Merged

Use multiple threads for refreshing feeds #7126

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0146e28
Refresh all feeds on multiple threads
0x082c8bf1 Apr 20, 2024
d5790a7
Add comment explaining why we're ignoring catch block
0x082c8bf1 Apr 20, 2024
9b61c59
Update notifications when downloading
0x082c8bf1 Apr 23, 2024
b9b6905
Change number of threads to 4
0x082c8bf1 Apr 24, 2024
bad3393
Rename for clarity
ByteHamster Apr 25, 2024
b19f244
Merge branch 'develop' into threaded-refreshing
ByteHamster Apr 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
67 changes: 45 additions & 22 deletions ...ervice/src/main/java/de/danoeh/antennapod/net/download/service/feed/FeedUpdateWorker.java
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@
import androidx.core.content.ContextCompat;
import androidx.work.ForegroundInfo;
import androidx.work.WorkManager;
import java.util.concurrent.TimeUnit;
import androidx.work.Worker;
import androidx.work.WorkerParameters;
import com.google.common.util.concurrent.Futures;
Expand Down Expand Up @@ -39,6 +40,8 @@
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;

public class FeedUpdateWorker extends Worker {
private static final String TAG = "FeedUpdateWorker";
Expand Down Expand Up @@ -127,36 +130,56 @@ private Notification createNotification(@Nullable List<Feed> toUpdate) {
.build();
}

private void updateNotification(List<Feed> toUpdate) {
if (ContextCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.POST_NOTIFICATIONS)
== PackageManager.PERMISSION_GRANTED) {
notificationManager.notify(R.id.notification_updating_feeds, createNotification(toUpdate));
}
}

@NonNull
@Override
public ListenableFuture getForegroundInfoAsync() {
return Futures.immediateFuture(new ForegroundInfo(R.id.notification_updating_feeds, createNotification(null)));
}

private void refreshFeeds(List<Feed> toUpdate, boolean force) {
while (!toUpdate.isEmpty()) {
if (isStopped()) {
return;
}
if (ContextCompat.checkSelfPermission(getApplicationContext(), Manifest.permission.POST_NOTIFICATIONS)
== PackageManager.PERMISSION_GRANTED) {
notificationManager.notify(R.id.notification_updating_feeds, createNotification(toUpdate));
}
Feed feed = toUpdate.get(0);
try {
if (feed.isLocalFeed()) {
LocalFeedUpdater.updateFeed(feed, getApplicationContext(), null);
} else {
refreshFeed(feed, force);
//Unchecked is used to prevent a race condition where a feed finishes downloading before
// we're done submitting all of the jobs
List<Feed> unchecked = new ArrayList<Feed>(toUpdate);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this copy actually needed? You are only updating the list in a synchronized block anyway. Is this maybe a leftover from an earlier experiment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be, because we are iterating over toUpdate and I guess we shouldn't remove items from a list we are currently iterating over.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's correct if it were to somehow manage to get to the remove before the for loop was done, we
d get an ConcurrentModificationException so the copy is there to prevent that.

updateNotification(unchecked);
ExecutorService executor = Executors.newFixedThreadPool(5);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 sounds a bit arbitrary. I think the default was 4 when we had parallel refresh before (which got removed after a rewrite of the download code)

for (Feed feed : toUpdate) {
executor.submit(() -> {
if (isStopped()) {
return;
}
} catch (Exception e) {
DBWriter.setFeedLastUpdateFailed(feed.getId(), true);
DownloadResult status = new DownloadResult(feed.getTitle(),
feed.getId(), Feed.FEEDFILETYPE_FEED, false,
DownloadError.ERROR_IO_ERROR, e.getMessage());
DBWriter.addDownloadStatus(status);
}
toUpdate.remove(0);
try {
if (feed.isLocalFeed()) {
LocalFeedUpdater.updateFeed(feed, getApplicationContext(), null);
} else {
refreshFeed(feed, force);
}
} catch (Exception e) {
DBWriter.setFeedLastUpdateFailed(feed.getId(), true);
DownloadResult status = new DownloadResult(feed.getTitle(),
feed.getId(), Feed.FEEDFILETYPE_FEED, false,
DownloadError.ERROR_IO_ERROR, e.getMessage());
DBWriter.addDownloadStatus(status);
}
synchronized (unchecked) {
unchecked.remove(feed);
}
if (!unchecked.isEmpty()) {
updateNotification(unchecked);
}
});
}
executor.shutdown();
try {
executor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);
} catch (InterruptedException e) {
//~300 years has elapsed
}
}

Expand Down