google / google/ExoPlayer

Provide thread safe way to modify custom SimpleCache evictor

Open
#8,628 5 comments 0 reactions 1 assignee Assigned to @marcbaechinger View on GitHub
enhancement
Dominant language
Java
Stars
21.9k
Forks
6k
PR merge metrics
No merged PRs in 30d

Description

Filed in response to: https://github.com/google/ExoPlayer/pull/8589.

Most custom player components are used by ExoPlayer on the playback thread. To modify such components in a thread safe way, we suggest that developers should use ExoPlayer's message passing functionality (`ExoPlayer.createMessage`) to deliver changes on the playback thread.

For a custom `CacheEvictor` injected into `SimpleCache`, this approach doesn't work. `SimpleCache` is accessed on multiple threads, so the important thing is which mutex you hold, not which thread you're calling on.

The change suggested in the pull request was not thread safe as proposed, because it allowed non-thread-safe access and modification of `leastRecentlyUsed`. Synchronizing all of the evictor's public methods doesn't work either, because this approach will cause deadlock problems where one thread holds the the `SimpleCache` mutex, another holds the evictor's mutex, with each needing to acquire the other.

To modify a `SimpleCache` evictor, the important thing is that the caller holds the `SimpleCache` mutex, so technically the change in the pull request is safe provided the new method is always accessed like:
```
synchronized (simpleCache) {
customEvictor.changeMaxBytes(newMaxBytes);
}
```
It's error-prone to rely on this, however, since this would break if someone (not unreasonably!) were to change `SimpleCache` to use an internal mutex in some future release of ExoPlayer.

I think ideally, `SimpleCache` would have a method to deliver a message through to the evictor whilst holding the correct mutex. So the application code could deliver an update safely using:
```
simpleCache.updateEvictor(messageType, payload);
```

This seems quite specific, but I think it's quite a specific case. `SimpleCache` doesn't have any other potentially-custom components that would reasonably require delivering messages to. The specificity also makes the API more obvious than what we'd be able to achieve with a general solution.

At the other end, there are really general solutions, such as `SimpleCache` letting you execute arbitrary code whilst holding the correct mutex. I think this would be quite hard for an application developer to discover and work out how to use correctly, however.

@tonihei - Any thoughts? Asking because IIRC you did most of the `PlayerMessage` work. Does my specific `updateEvictor` proposal, and a new `CacheEvictor.handleMessage(int, Object)` with a default no-op implementation, sound reasonable? Feel free to assign back after providing input :).

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.