Skip to content

session: Avoid ForegroundServiceDidNotStartInTimeException... #2626

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.

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

nift4
Copy link
Contributor

@nift4 nift4 commented Jul 17, 2025

...and throw more useful exceptions instead.

This will however not throw exceptions where foreground service
start is not absolutely required, and keep logging warnings in
these cases instead.

Issue: #2591
Issue: #2622

(This PR includes PR #2256 because they conflict. It would probably make sense to merge #2256 first.)

@nift4
Copy link
Contributor Author

nift4 commented Jul 25, 2025

As requested earlier, a brief reasoning of the design choices:
Throwing and giving up with a clear error message seems to be the best we can do, because:

  1. there's no way to start the service from a broadcast reciever without the system enforcing a foreground service will start after we tell it to (bindService hence MediaController isn't supported from a broadcast reciever, and startService without foreground will - if I understand correctly - not transfer the foreground allowlist to our service)
  2. if the system enforces the service will start after we tell it to, shouldStartForegroundService() should be overwritten for every app that supports logging out / empty playlists / etc, but it's not very discoverable
  3. the consequence of not doing that is a cryptic ForegroundServiceDidNotStartInTimeException

1 is a hard fact and we can't do anything about it, 2 is more of a documentation problem and it seems external contributors can't edit those anyway, but 3 is possible to address - and it will give a concrete improvement for everyone suffering from ForegroundServiceDidNotStartInTimeException without understanding the reason, as it makes it practically impossible for the library to generate one now. I myself had ForegroundServiceDidNotStartInTimeException in Play Console because I didn't realize shouldStartForegroundService() is required, but with this patch there'd be a useful explainer instead.

I also don't see any disadvantage of this patch for SDK>=31, and I consider the added sanity check on <31 an advantage because developers that don't always test on Android 12+ will still get notified about the fact their code is wrong.

nift4 added 4 commits July 29, 2025 17:15
...and throw more useful exceptions instead.

This will however not throw exceptions where foreground service
start is not absolutely required, and keep logging warnings in
these cases instead.

Issue: androidx#2591
Issue: androidx#2622
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants