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.

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

[Android] Remove AndroidServiceStartType and update docs #2481

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Levi-Lesches
Copy link
Contributor

@Levi-Lesches Levi-Lesches commented Dec 2, 2024

See #2479 for details on why this was needed (the option wasn't doing anything to begin with)

Fixes #2479
Fixes #2446
Fixes #2298
Fixes #2407

@Levi-Lesches
Copy link
Contributor Author

Seems the CI is failing because the example app is still using the old plugin syntax. I can open a PR for that sometime soon

@EPNW-Eric
Copy link

Looks good to me 👍

@martin-bertele
Copy link
Contributor

Seems the CI is failing because the example app is still using the old plugin syntax. I can open a PR for that sometime soon

fixed in here
#2482

@leaf-node
Copy link

Wait, I think we shouldn't remove the AndroidServiceStartType option completely, because there are two valid values for this setting: the START_NOT_STICKY and START_REDELIVER_INTENT. The first would cause the foreground service to stop entirely if the system needs to pause that activity. And the latter would schedule the foreground service to resume when resources are available, correct? I don't think it's right to only allow use of the START_NOT_STICKY mode, because many people may want their notifications to persist.

If a value has to be hard-coded, it should be the START_REDELIVER_INTENT option so foreground services will continue to be reliable on Android. But I can see a use for both, so I suggest keeping the option in place, setting the default to START_REDELIVER_INTENT, and removing the START_STICKY option, because that one is not supported by this library.

@Levi-Lesches Levi-Lesches marked this pull request as draft December 3, 2024 23:17
@MaikuB
Copy link
Owner

MaikuB commented Jan 12, 2025

Not done testing on this but inclined to agree with @leaf-node and removing it seems more risky. I would suggest making this a required parameter. This way developers spend some time looking into the option that fits their scenario and the results are the consequence of the choice they made.

@leaf-node
Copy link

@MaikuB, Levi offered more technical detail about this issue here: #2479, and I agree with a lot of what Levi says. My understanding is that the current feature is not working as intended, as the service is not resumed after it is paused by the OS. Ideally this could be resolvable for this package, perhaps by setting a callback that is called by the package when the foreground service is resumed by the OS.

I'm currently working on creating a manual foreground service with my app's Kotlin code, which may give me some insight into how a foreground service works. I don't yet know enough to resolve / patch the issue on my own, but I might be able to help.

Also, as Levi points out, there are other modules that try to provide foreground services on Android, with some different design decisions that don't fit my use case exactly. Many of them integrate with this package, and depending on how things go, I might create another alternative if the foreground service feature is removed from this package.

@MaikuB
Copy link
Owner

MaikuB commented Jan 12, 2025

@leaf-node thanks for the response. Based on my understanding, my preference would be to avoid removing the functionality as there could be cases where there are existing apps that choose to use this plugin to show the notification and start the operation independently. The one that seems to be problematic is the one tied to START_STICKY

By the way, I don't think I've seen it mentioned but what is your actual use case? I saw you mentioned that picking START_NOT_STICKY fixes the issue for you and makes sense as there wouldn't be a null intent. Even if a callback was being supported, avoiding that crash would still be needed. If you needed a callback so that work can resume on a restart, flutter_foreground_task that @Levi-Lesches shared in #2479 looks to be more appropriate. For a callback to be supported, the only way that Dart code can be invoked in this scenario is through the use of background isolates. Background isolates are what allows Dart code to be run when the app isn't running and is the same mechanism used for notification actions that don't require interacting with a running app. flutter_foreground_task appears to support that already and would even appear to provide functionality to communicate back to the main isolate in the scenario and app is running

@leaf-node
Copy link

@MaikuB The use case for my app is to fetch server alert statuses from third-party remote servers, and to notify the user if a new alert is found. I'm not using push notifications, so I create a foreground service, allowing my app to fetch from the network and create alert notifications.

As for START_STICKY vs START_NOT_STICKY, if I could, I would prerfer former for my app, because (if I understand correctly) it will restart the app at the .ForegroundService class, but the Kotlin / Java glue code would have to function without access to the original intent. The START_NOT_STICKY option worked for me, but only in the sense that it didn't trigger a crash. START_REDELIVER_INTENT would be an ideal option, because with more work, it would allow the service to resume after the OS stops my app to clear up memory, etc. Currently, this notification package doesn't seem to recreate the foreground service after it is stopped.

As for background isolates, I'm pretty sure that they are implemented as separate threads on the same FlutterEngine, and platform code is capable of creating and stopping multiple of those. The background isolates exist in a FlutterEngine, so when the app stops, the flutter engine is stopped, and so are the isolates. When the service is running as a foreground service, I'm pretty sure that's mostly a notification that prolongs the life of the app even when not directly used. And the declaration of the foreground service in the AnroidManifest.xml file is used as an entry point for the OS to re-create a foreground service that was temporarily, but really and completely, stopped.

It's possible to create a FlutterEngine that calls into a specific entrypoint in the Dart code, skipping the main() function. So I'm trying to leverage this in my app. When I use that approach, the UI is not run, and the app process, including its service, uses less memory until the user launches the app. At that point, I kill the FlutterEngine that doesn't have a UI, because that Flutter engine was created with an Intent that doesn't offer the permission to run UI code.

And the sticky options are to tell the OS whether an app's foreground service should be restarted after it is killed by the OS for memory cleanup, etc. I suppose in some cases, it might be useful to have a foreground service that runs for a while, then somewhat arbitrarily stops, but in my case I would want something that always, or usually, comes back, even if it takes a long time.

I should also note that there is a 6 hour limit for foreground services in a 24 hour period, until the user interacts with the UI of the app again, unless the app uses a special permission that is harder to get into the Play Store. Some packages for background work operate by scheduling a series of timers. Between each tick, the app is not running, which might help with the time limit for the app.

I'm still new to this topic, so it's not all 100% clear to me, but I'm learning. @Levi-Lesches does that match what you understand about this topic?

@MaikuB
Copy link
Owner

MaikuB commented Jan 12, 2025

@MaikuB The use case for my app is to fetch server alert statuses from third-party remote servers, and to notify the user if a new alert is found. I'm not using push notifications, so I create a foreground service, allowing my app to fetch from the network and create alert notifications.

What triggers this to run? Putting aside that there's an issue with START_STICKY for this plugin, currently I can't tell why would be preferred as well. Other thing is how do you handle this on iOS?

It's possible to create a FlutterEngine that calls into a specific entrypoint in the Dart code, skipping the main() function. So I'm trying to leverage this in my app. When I use that approach, the UI is not run, and the app process, including its service, uses less memory until the user launches the app. At that point, I kill the FlutterEngine that doesn't have a UI, because that Flutter engine was created with an Intent that doesn't offer the permission to run UI code.

What you've described here is actually referring to background isolates and is what this plugin already leverages for handling notification actions that is required to trigger a callback on a background isolate. The reference in the official docs can be found here , which in turn refers to this. Have you tried flutter_foreground_task or did you find something that it's missing? Conscious that you may end up wasting time implementing a lot of what it already does that if it was missing something that you're better off forking it as a basis to add in what is missing

@Levi-Lesches
Copy link
Contributor Author

@leaf-node

Wait, I think we shouldn't remove the AndroidServiceStartType option completely, because there are two valid values for this setting: the START_NOT_STICKY and START_REDELIVER_INTENT

...

START_REDELIVER_INTENT would be an ideal option, because with more work, it would allow the service to resume after the OS stops my app to clear up memory, etc. Currently, this notification package doesn't seem to recreate the foreground service after it is stopped.

@MaikuB

Based on my understanding, my preference would be to avoid removing the functionality as there could be cases where there are existing apps that choose to use this plugin to show the notification and start the operation independently. The one that seems to be problematic is the one tied to START_STICKY

So we all agree that START_STICKY is not useful in its current form, and is almost always causing null errors.

Now, about START_REDELIVER_INTENT: I haven't gotten around to testing this yet either (hence why this is still a draft), but my main concern is that this isn't doing what we want either. The intention is that the OS should show the notification again and re-start the foreground service, along with any work it was doing. But, as far as I can tell, that's not what's happening. It seems the foreground service's native side (ie, Java) is just starting the foreground service, showing the notification, and not doing any work. Because, as you mentioned, showing the notification is done (ultimately) in Java, and doing the work is done in Dart. And there's no link between the two like package:flutter_foreground_service or others have.

If that's correct, then we should remove this option because it sounds like something useful is happening, and a notification means the user thinks something useful is happening, but nothing is happening at all. Again, haven't tested, but the code in ForegroundService.java is so short and contains no references to Dart code of any kind. It just shows the notification and quits immediately. This means that no matter what option you pass in today, START_NOT_STICKY is the real behavior, and the APIs should reflect that.

@leaf-node
Copy link

What triggers this to run? Putting aside that there's an issue with START_STICKY for this plugin, currently I can't tell why would be preferred as well. Other thing is how do you handle this on iOS?

@MaikuB I'm running a timer in Dart that then calls code to perform an HTTP request each time data is fetched by the app. I don't have an iOS version, largely due to a lack of a foreground service feature there. As for flutter_foreground_task, it schedules work every 15 minutes, but I'd like more flexibility in the timing of work, and the reliability that a foreground service provides. Although, that plugin does support iOS, and a foreground service may have a limited lifespan. You make a good point, and hopefully I can take some of what I've learned and make some pull requests against existing plugins.

This means that no matter what option you pass in today, START_NOT_STICKY is the real behavior, and the APIs should reflect that.

@Levi-Lesches Are you thinking that it would currently make sense to require the startNotSticky parameter when launching a foreground service, for the sake of clarity, but then once there's support for calling back into Dart code from Java, it would make sense to allow the startRedeliverIntent option again?

@Levi-Lesches
Copy link
Contributor Author

Levi-Lesches commented Jan 17, 2025

Are you thinking that it would currently make sense to require the startNotSticky parameter when launching a foreground service, for the sake of clarity,

Yes, since anything else would be misleading, but

then once there's support for calling back into Dart code from Java, it would make sense to allow the startRedeliverIntent option again?

I'm not sure this is ever going to happen, as we've mentioned that other packages like flutter_foreground_task do this better (and not just for Android), and I think it's better to leave it to them and consider it out-of-scope for this plugin, if @MaikuB agrees.

So I think the option should just be removed permanently, and startForegroundService() can be considered "tell Android to elevate this app to foreground status and show a fancy notification for it", and anything else would require using a different package. That keeps this plugin focused only on the notifications side of things.

@MaikuB
Copy link
Owner

MaikuB commented Jan 17, 2025

I'm running a timer in Dart that then calls code to perform an HTTP request each time data is fetched by the app. I don't have an iOS version, largely due to a lack of a foreground service feature there. As for flutter_foreground_task, it schedules work every 15 minutes, but I'd like more flexibility in the timing of work, and the reliability that a foreground service provides.

@leaf-node thanks for the explanation. Have I missed something that's perhaps only in the implementation details or API docs? I've not taken a look at either in depth but the only mention of 15 minutes I've seen in the readme is specific to iOS and you don't have an iOS version for it be applicable

@leaf-node
Copy link

@leaf-node thanks for the explanation. Have I missed something that's perhaps only in the implementation details or API docs? I've not taken a look at either in depth but the only mention of 15 minutes I've seen in the readme is specific to iOS and you don't have an iOS version for it be applicable

@MaikuB Yeah, it looks like flutter_foreground_task supports other intervals, so I may have been thinking of a different plugin. I investigated a few, so I'm not sure why I passed on that one. Maybe it was because the isolate running my app's model needs to keep running while the UI is visible, even if the user chooses to disable all notifications, including the foreground service one. It should be possible to work around that issue, but I thought that I wouldn't need much Kotlin code in a custom approach. I ended up with almost 300 lines of Kotlin, which isn't too bad, and I feel like I learned from the process, so I'm happy with the result. We'll see about working out any subtle bugs though. : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment