DownloadManager task sync creates too many concurrently-running threads for moderate amounts of STATE_RESTARTING or STATE_REMOVING downloads in index

[stoyicker]

ExoPlayer Version

2.18.0

Devices that reproduce the issue

Pixel 2 emulator API 31
TCL 10 SE TCL UI 3.0.7.K2V (API 30)

Devices that do not reproduce the issue

None found

Reproducible in the demo app?

Yes

Reproduction steps

Simple setup:

1. Check out https://github.com/stoyicker/mwe-exoplayer-downloadmanager-oom/tree/7d36b7c2e3cff7956b70de9700d192ca5874acec It is an empty app using exoplayer 2.18.0 set up to create a downloadmanager with an index that will get problematic downloads.
2. Choose an appropriate number of downloads with problematic state to add to the index. For this setup, my emulator would crash with 1.5k and my device with 2k. Using values too low for your setup will result in the expected result.
3. Run the app.
4. When the app shows the black screen stating it's ready, tap it. This will start adding downloads with one of the states mentioned in the title to the index to simulate the problematic situation, and when it's done it'll change the screen color to red and proceed to create a DownloadManager with this index.

Demo app setup:

1. Check out https://github.com/stoyicker/ExoPlayer/tree/a2e23ae8f700556c9c6cc6f08e377fb81a2621ae to recreate scenario. It is the tag for 2.18.0 with a customized download index that will get problematic downloads added.
2. Choose an appropriate number of downloads with problematic state to add to the index. For this setup, my emulator would crash with 4k (although I noticed that smaller amounts would print the exception as a log instead of crashing, but I couldn't pinpoint from where) and haven't tested my device. Using values too low for your setup will result in the expected result.
3. Run the app.
4. Wait for the list of demos to render, which will mean that the index has been filled with the requested amount of downloads.
5. Wait a few seconds after the list of demos renders. Less than 30s should do.

Make sure you always uninstall the app or clear its data when trying anew to avoid picking up the old index. Also note that this applies not only on initialization, but on any calls that trigger a call to syncTasks with the appropriate amount of downloads in the problematic states.

Expected result

Simple setup:

When the DownloadManager is initialized (as in, the activeTask map filled because of the syncTasks call in initialize has been emptied, not when the callback has been invoked), the screen will change to blue, so it is expected that after a few seconds this happens.

Demo app setup:

The app continues to run after 30s of wait (starting after the list has been drawn).

Actual result

Simple setup:

You never get to the blue screen. Instead, the app crashes with the exception below.

Demo app setup:

It crashes because of the exception below.

Example stacktrace:

java.lang.OutOfMemoryError: Failed to allocate a 131088 byte allocation with 80744 free bytes and 78KB until OOM, target footprint 536870912, growth limit 536870912
        at com.google.android.exoplayer2.upstream.cache.CacheWriter.<init>(CacheWriter.java:78)
        at com.google.android.exoplayer2.offline.ProgressiveDownloader.<init>(ProgressiveDownloader.java:82)
        at com.google.android.exoplayer2.offline.DefaultDownloaderFactory.createDownloader(DefaultDownloaderFactory.java:83)
        at com.google.android.exoplayer2.offline.DownloadManager$InternalHandler.syncRemovingDownload(DownloadManager.java:1062)
        at com.google.android.exoplayer2.offline.DownloadManager$InternalHandler.syncTasks(DownloadManager.java:986)
        at com.google.android.exoplayer2.offline.DownloadManager$InternalHandler.initialize(DownloadManager.java:816)
        at com.google.android.exoplayer2.offline.DownloadManager$InternalHandler.handleMessage(DownloadManager.java:737)

I'm fairly certain this is due to the amount of tasks started from syncRemovingDownload not being capped, for example such as the the ones from syncQueuedDownload which are capped by the amount of parallel downloads. I was thinking of drafting a solution where Task is changed to extend Runnable instead of Thread and then a new setter is introduced, akin to that for max parallel downloads, to control the amount of threads in an executor that we submit tasks to, but wanted to run the issue by you before. Note that I understand that 2k may seem a large amount of items to have removing, but I don't think it is a justification to consider this a corner-case given that not only does it reproduces consistently in an unrealistically simple app, but also in the demo app (which sets largeHeap to true) with a not-much-higher-number.

Media

N/A

Bug Report

• You will email the zip file produced by adb bugreport to [email protected] after filing this issue.

[ojw28]

Thanks for the detailed bug report!

I'm fairly certain this is due to the amount of tasks started from syncRemovingDownload not being capped

That sounds likely, and yes, capping it is probably a good idea. I don't think there needs to be a setter to configure the value; we probably just need to pick a suitable value internally (which might be 1).

[ojw28]

I was thinking of drafting a solution where Task is changed to extend Runnable instead of Thread and then a new setter is introduced, akin to that for max parallel downloads, to control the amount of threads in an executor that we submit tasks to, but wanted to run the issue by you before.

I would suggest you solve the problem in the simplest way possible, which is probably to keep Task how it is and to introduce a limit analogous to maxParallelDownloads (except that there's no need for a setter). Would you be able to put together a pull request for this?

Switching DownloadManager to use executors (which would ideally be injectable) is also a good idea, but I think that should probably be decoupled from fixing the immediate problem.

[stoyicker]

sure thing

[ojw28]

@stoyicker - Thanks! We will await a pull request. Reassigning to @marcbaechinger who will probably be the person to review it once it's available.

[stoyicker]

@ojw28 The PR is open already, but it's on the media3 repo, as I opened another (unrelated) one here a few days ago was asked to take it there instead, so I assumed the same would apply for this one. Do you want me to open it here instead?

[ojw28]

Ah, that's fine, thanks! Reassigning to @tonihei in that case, since the PR is assigned to him.