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

[flutter_local_notifications] Fix crashes related to Android foreground service start type #2475

Closed
wants to merge 1 commit into from

Conversation

EPNW-Eric
Copy link

See #2446

…ce start type to startRedeliverIntent and warn about startSticky
@Levi-Lesches
Copy link
Contributor

LGTM for changing the default and the docs, but I still think it's wrong that we don't try to handle the error condition at all.

I know you mentioned that a crash would help people realize that they're missing behavior, but my question is, what should we be doing for users when the intent is null? From what I understand, this isn't an error condition, just no work for us to complete at this point, so it should be safe to return. As far as I know, we never claimed to be able to restart work after sudden termination, so it's not like people are expecting this from us.

@EPNW-Eric
Copy link
Author

So onStartCommand requires us to return one of the start flags START_STICKY, START_NOT_STICKY and START_REDELIVER_INTENT. Usually we are getting this from the intent, but without intent what should we return? Returning START_STICKY (which was most likely the start flag before system termination of the service) would return in an endless loop? So return START_NOT_STICKY instead? And maybe even call stopSelf?

@Levi-Lesches
Copy link
Contributor

Levi-Lesches commented Nov 28, 2024

From START_REDELIVER_INTENT:

This Intent will remain scheduled for redelivery until the service calls stopSelf(int) with the start ID provided to onStartCommand(Intent, int, int)

Good point, it seems we must call stopSelf() when we're done or else the app may end up repeating the service infinitely. Seems we already have stopForegroundService which calls stopService, which should accomplish the same as stopSelf.

So, given all that:

  • if we use startSticky, we need to handle the null case. I can't think of a good thing to do here besides returning.
  • if we use startNotSticky and the app was suddenly terminated while working, it won't restart automatically
  • if we use startRedeliverIntent, it won't go away unless the dev calls stopForegroundService

Most importantly though, I just realized... a real service is meant to do real work. This plugin only uses services to implement permanent notifications. That means the real work is still being done in Dart, which should really use startService and stopService as needed. That also means restarting the service won't have any actual effect besides showing a notification telling the user something is happening -- even though it isn't (evidenced by onStartCommand only showing the notification and returning, even in the best case).

We should probably change the default to startNotSticky instead of startRedeliverIntent since that's the only mode we actually support. We should mark the enum and parameter as @deprecated, as they will only crash or in the best case, do nothing. If someone is trying to use these to restart real work, they should get a warning that it isn't doing that, and they should be using a different package like flutter_foregound_task or write their own native code, which will allow them to implement real logic. We should also change the documentation and README to reflect this.

@EPNW-Eric
Copy link
Author

I was asking @leaf-node in #2446 to test if using START_NOT_STICKY can achive the desired behaviour. While I agree there is no use for START_STICKY I would await the answer before proceeding here, maybe we get some additional insights if START_REDELIVER_INTENT is not usefull.

@EPNW-Eric
Copy link
Author

See #2479

@EPNW-Eric EPNW-Eric closed this Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants