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

Unnecessary killing and starting of isolates when app is backgrounded #417

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kumulynja
Copy link
Contributor

As far as I know, isolates are backgrounded and resumed together with the main app thread, so I think it is not necessary to kill and resume them explicitly ourselves. Normally they will be paused when the app is backgrounded and resumed when it is foregrounded again.

…uming since isolates are backgrounded and resumed together with main thread
@kumulynja
Copy link
Contributor Author

kumulynja commented Jan 7, 2025

@DanGould I might be wrong here, but also tested it and isolates seem to resume automatically after the app is backgrounded and foregrounded again. Did you notice it was needed to resume them explicitly? Or was there any other reason you killed and resumed them explicitly on app lifecycle state changes?

@kumulynja
Copy link
Contributor Author

kumulynja commented Jan 7, 2025

@i5hi might be needed to test this on a real device still. I can only test on emulator for the moment. Would be some nice help if you could see if sessions are active again after backgrounding and resuming the app with this branch.

@DanGould
Copy link
Contributor

DanGould commented Jan 7, 2025

My understanding is that processing on the main isolate can freeze the UI, (like #403 which might be caused by the main isolate being blocked by the process starting a bunch of isolates) and since those sessions are all waiting for responses on web requests, my concern was that either many concurrent requests or response handling sessions could block the main isolate and cause jank in the UI. This remains my primary concern.

If this design is not necessary I am by no means tied to it. The isolates are way more complicated than having everything on main.

The other reason spawning isolates per session made sense to me was because I assumed (wrongly?) that in order to have concurrent requests in the background they would need to be spawned in isolates. My intuition was that we want them to run concurrently and if we wait for the OS to stop them, we might get penalized for not handling background task termination properly. I am informed from studying iOS background processing tasks, which I believe carries over in large part to Android, but I may be a few years out of date or misunderstand how Flutter abstractions translate to OS specifics. Simple searches are telling me that Dio HTTP operations are generally automatically cancelled when the app goes to the background, which may make this second background-concerned rationale for isolates insufficient. But I'm not sure how concurrent long-polling requests would behave.

The background processing concern I have is from iOS, where if you didn't explicitly clean up the main process the OS will force kill it after some time. However, every time the OS kills your process for you you get penalized in your ability to get background process time / send push notifications, so I beleive the hygienic thing to do is to leave the isolates. Eventually, we want to use the specific OS background processing APIs to allow it to happen in the background for more liveliness. We probably want to use the cross-platform equivalent to BGAppRefreshTask for this.

@kumulynja
Copy link
Contributor Author

kumulynja commented Jan 7, 2025

My understanding is that processing on the main isolate can freeze the UI, (like #403 which might be caused by the main isolate being blocked by the process starting a bunch of isolates) and since those sessions are all waiting for responses on web requests, my concern was that either many concurrent requests or response handling sessions could block the main isolate and cause jank in the UI. This remains my primary concern.

If this design is not necessary I am by no means tied to it. The isolates are way more complicated than having everything on main.

The other reason spawning isolates per session made sense to me was because I assumed (wrongly?) that in order to have concurrent requests in the background they would need to be spawned in isolates. My intuition was that we want them to run concurrently and if we wait for the OS to stop them, we might get penalized for not handling background task termination properly. I am informed from studying iOS background processing tasks, which I believe carries over in large part to Android, but I may be a few years out of date or misunderstand how Flutter abstractions translate to OS specifics. Simple searches are telling me that Dio HTTP operations are generally automatically cancelled when the app goes to the background, which may make this second background-concerned rationale for isolates insufficient. But I'm not sure how concurrent long-polling requests would behave.

The background processing concern I have is from iOS, where if you didn't explicitly clean up the main process the OS will force kill it after some time. However, every time the OS kills your process for you you get penalized in your ability to get background process time / send push notifications, so I beleive the hygienic thing to do is to leave the isolates. Eventually, we want to use the specific OS background processing APIs to allow it to happen in the background for more liveliness. We probably want to use the cross-platform equivalent to BGAppRefreshTask for this.

Yes, you are right about all of that. This PR is not a change in the current design with isolates. It just removes the explicit killing and resuming of them on app lifecycle changes. The isolates we are spawning right now are not really "background isolates" in the sense that they currently do not keep running when the app is moved to the background. The current isolates that are spawn only run together with the app in the foreground. Whenever the app is moved to the background, they are automatically paused together with the main thread's lifecycle. And whenever the app comes back to the foreground the isolates are also automatically resumed. They will not be killed by the OS as long as the app itself is not killed. They are part of the main app process.

The background tasks/isolates that have limited time and resources to run and that can get killed by the OS are background services that are really run in an OS process separate of the main app. So as long as we don't have "real" background isolates/services that run even when the main app is not running, we shouldn't worry about the OS killing them yet. That's why this PR just removes the killing and resuming of the current isolates, since currently this is not necessary from what I understand about isolates and background services (but I might be wrong).

As you mention, if we eventually use BGAppRefreshTask and WorkManager, we will indeed have those concerns, but then the isolates will probably not run like this in the main app anymore, but in their own processes, which we should then indeed design well to consume as few resources as possible and without any guarantee of assigned time-to-run in mind.

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