Create `BGContinuedProcessingTask` to upload media

[crazytonyli]

Description

Here is a recording.

https://github.com/user-attachments/assets/2c091e6c-d1de-4b74-8b92-9d02f7ca9e8c

[wpmobilebot]

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Configuration	Release-Alpha
	Build Number	29655
	Version	PR #24891
	Bundle ID	org.wordpress.alpha
	Commit	1ce20c043bc3cf262fe92644e7e7e59f84a01360
	Installation URL	0uf9jjmkolktg

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

[wpmobilebot]

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Configuration	Release-Alpha
	Build Number	29655
	Version	PR #24891
	Bundle ID	com.jetpack.alpha
	Commit	1ce20c043bc3cf262fe92644e7e7e59f84a01360
	Installation URL	15cpksdkb4hfo

Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

[wpmobilebot]

Version 26.4 has now entered code-freeze, so the milestone of this PR has been updated to 26.5.

[crazytonyli]

I'm concerned about how many errors we're ignoring.

@jkmassel Do you mean the try? statements on the Core Data calls? They are actually intentional. Those statements should only throw errors when the media object is deleted. That's related to your comments later about deleting uploaded images. If the Media object is deleted, we don't need to count them in the progress/status updates.

I didn't add code to observe Core Data changes to immediately handle deleting uploading images, because the Core Data notifications get fired pretty frequently. That's one issue I noticed in the UI, where the uploading status is not updated immediately when deleting uploading images.

[crazytonyli]

I strongly suggest simply putting it on the main actor

@kean I had a go myself and noticed that it's very easy to make the mistake of calling MainActor bound functions from background threads. Take the taskCreated function as an example, it should be running in the MainActor since the class is marked as @MainActor. But you can call it without wrapping it in a Task, too. The compiler does not complain about it, but at runtime, it crashes. This kind of error can probably be caught at compile time by complete strict concurrency checks, which is not set in the app target.

I spent quite a while trying to avoid that issue: declaring the type as @MainActor class, but ensuring its functions are only called from the main thread. I couldn't make it work.

I opened a prototype PR just to show how much simpler it is if it's on main – it eliminates 12 awaits.

I agree that we should avoid jumping between actors if we can, especially in this particular case, where we have to use the MainActor frequently. However, fewer awaits is not necessarily better. Like the example I used above, we'd want the await keyword to be there when calling taskCreated, otherwise the function won't be executed from the main thread.

[kean]

but ensuring its functions are only called from the main thread. I couldn't make it work.

If there is some pre-concurrency code, which I assume BGTaskScheduler, it'll might need to use Task { @MainActor. It's an exception.

very easy to make the mistake of calling MainActor bound functions from background threads

It is what MainActor is generally for. There may be some exceptions with pre-concurrency code, but these are just exceptions. I don't see how it would translate into "therefore, we should never use MainActor for synchronization"? It enforces isolation extremely well.

However, fewer awaits is not necessarily better

It's easier to follow code that has less concurrency. If it's possible to eliminate 12 async calls in a ~100 lines of code, it's a big difference. If I needed to debug and fix something in ConcreteMediaUploadBackgroundTaskScheduler, I would probably start by simplifying it – it is hard to follow in the current version.

[crazytonyli]

Maybe using a practical example can help illustrate my concerns.

Let's use this one single happy path as the test method: Upload an image from the Media screen, the OS shows an "Uploading Media" banner, updates the progress, and finally dismisses the banner once uploaded.

Obviously, the test works on this PR branch.

I have refactored the code to make everything run on the main thread in the media-upload-using-bgtask-mainactor branch. The code is very similar to your POC code in https://github.com/wordpress-mobile/WordPress-iOS/pull/24943. Your PR does not work, so I pushed my own changes.

The test works on the media-upload-using-bgtask-mainactor branch too.

If you check out the diff, you'll see there is no Task { ... } and await calls. Everything is sync-ed and runs on the main thread, which is, again, just like your POC code. But if you remove two places that call DispatchQueue.main, and perform the test again, the app crashes due to Core Data concurrency issues. BTW, this is one of the reasons that your POC code does not work.

My concern is that Xcode does not warn us about removing DispatchQueue.main code changes, even though we have declared @MainActor, and the functions are supposed to be running on the main thread. That means, we have to make the conscious effort of writing threadsafe code: calling the MainActor-bound functions from the main thread (using DispatchQueue.main, wrapping in Task { ... }, etc), which is no different than before the "Swift structured concurrency" era.

That brings us to this point: if you reset your working copy to the media-upload-using-bgtask-mainactor branch, and remove the @MainActor declaration, the test continues to work. The @MainActor declaration does almost nothing to the compilation time behaviour and runtime behaviour. The fact that the code is thread-safe is due to the code being carefully written to call certain functions on the main thread, not because the class is declared as @MainActor.

If we have the complete strict concurrency checks turned on in the app target, it's a different story. I'd expect the compiler to warn us about the code changes of removing DispatchQueue.main. And, it'd be much safer to use @MainActor then.

[crazytonyli]

It's easier to follow code that has less concurrency. If it's possible to eliminate 12 async calls in a ~100 lines of code, it's a big difference.

Yes, I agree. Readability is important. I'm happy to push the changes in media-upload-using-bgtask-mainactor onto this branch, and make this new class do things the old, simple, and "boring" way, and stop using Swift actor, async, etc. The downside is, of course, we have to be careful about calling functions from the expected thread.

[crazytonyli]

In terms of features, I'm not sure it should be shown for quick operations like uploading a site image or a featured image for a post – it may be too overwhelming

@kean @jkmassel Regarding this, I feel like it's too much to show the banner immediately after selecting images to upload, too. In the latest commit, I have changed to only show the banner when the app goes to the background. That means we can get extra background time for the uploads, at the same time, we don't confuse the user with the new banner when the app is active. Let me know what you think.

[kean]

expect the compiler to warn us about the code changes of removing DispatchQueue.main.

Sure, it is designed to work with Swift Concurrency. If you have legacy APIs with closures that do not enforce actor isolation, you will be able to call syncronous methods on a MainActor-isolated class without warnings. You have to check it yourself and add runtime checks. The reason actor works is because every method on it is async, so you have to have an async context.

I suggested (as a nit and after approval) to isolated it on main mainly to reduce the number of async calls to make the code easier to follow, and not necessarily for the compile-time checks. I understand the appeal of actors that they guarantee isolation, but, to me, it doesn't seem like this advantage is bigger than the disadvantages of making so much of the code concurrent and harder to follow. It's pretty easy to ensure the few places where DispatchQueue.main are necessary are correct.

he old, simple, and "boring" way,

Nothing beats the good old "just synchronize everything on main thread" 😃

Btw, not so well-known fact about actor: it does not guarantee the order in which async tasks schedule on it are executed. So it's not a one-to-one replacement for a serial GCD queue. I don't know if it's necessarily a problem for ConcreteMediaUploadBackgroundTaskScheduler, but it adds fuel to the fire by making it even harder to reason about async methods in actors. They do guarantee isolation. They do no guarantee that the tasks will execute in the order they were "scheduled". You end up with progress, status, context changes updates all being executed out of the order in which they arrived.

[crazytonyli]

@kean I have merged the media-upload-using-bgtask-mainactor branch into this PR.

[sonarqubecloud[bot]]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[wpmobilebot]

Version 26.5 has now entered code-freeze, so the milestone of this PR has been updated to 26.6.