MediaController.releaseFuture() cancellation race

[bubenheimer]

Media3 Version

Media3 1.0.1

Devices that reproduce the issue

N/A

Devices that do not reproduce the issue

No response

Reproducible in the demo app?

Not tested

Reproduction steps

MediaController.releaseFuture() seems to have a cancellation race that could leak the controller when the future gets done between isDone() and cancel():

media/libraries/session/src/main/java/androidx/media3/session/MediaController.java

Lines 520 to 532 in 3c01488

	public static void releaseFuture(Future<? extends MediaController> controllerFuture) {
	if (!controllerFuture.isDone()) {
	controllerFuture.cancel(/* mayInterruptIfRunning= */ true);
	return;
	}
	MediaController controller;
	try {
	controller = controllerFuture.get();
	} catch (CancellationException | ExecutionException | InterruptedException e) {
	return;
	}
	controller.release();
	}

Expected result

N/A

Actual result

N/A

Media

N/A

Bug Report

• You will email the zip file produced by adb bugreport to dev.exoplayer@gmail.com after filing this issue.

[tonihei]

All of the methods of MediaController are supposed to be called from the specified application thread (see https://developer.android.com/reference/androidx/media3/session/MediaController#threading-model), including releaseFuture. The Future itself is also only marked as done on the same thread... so the case you describe shouldn't happen? Did you see this happen somehow and was it caused by using a different thread for calling releaseFuture? If so, I'd be curious to hear if the different thread was needed or more natural to use in your app.

[bubenheimer]

Ah, right, I think I looked at that earlier and decided to go with it for that reason (release() must be called on the right thread). The intended contract is not obvious, though, as releaseFuture() is static and not directly tied to the specified applicationLooper. If it does get a hold of the controller without cancelling first, it would check if it's on the right thread, otherwise it would not.

To validate correctness I'd need to find the internals of where the future is marked done.

Perhaps the javadoc could clarify the contract. Also, can't you skip the isDone() check and just test the return value of cancel()? This would seem to address the unclear correctness part. Or would that hang if everything runs on the same thread that is also a looper thread?

[bubenheimer]

The reason I tripped over this was my MediaSessionService leaking, so I did a code inspection. I just use the main thread at the moment, on the Controller side.

[tonihei]

Thanks for the suggestion - we can mention it in the docs (and use future.cancel only without the additional isDone check)