[image_picker] Move disk accesses to background thread

[JeroenWeener]

The plugin accesses the disk on the UI thread at several occasions as reported by flutter/flutter#91393. These instances can easily be found by running the plugin with StrictMode enabled. The occasions that are highlighted are when determining the application's cache directory and when fetching SharedPreferences.

By running method channel invocations on a background channel using Pidgeon's @TaskQueue() annotation and deferring said IO operations to the moment where they are actually needed, this PR makes sure the plugin no longer accesses the disk from the UI thread.

This PR is a follow-up to #3506

• Fixes ImagePicker should avoid making I/O operations on the platform thread flutter#91393.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

[gmackall]

Thanks for the contribution! This mostly looks good to me, my only comment (and one it looks like @stuartmorgan raised previously), would be that it may make sense to refactor so that instead of making the calls to getCacheDir and getSharedPreferences lazy, they are still done ahead of when they are needed but on another thread. I think either way it would make sense to land this first though and have that be a follow up, so this LGTM!

@stuartmorgan have your thoughts on this changed since that initial discussion? If not I can make an issue to track. Also will hold off on autosubmit to give you time to do a secondary review.

[JeroenWeener]

Thanks for the contribution! This mostly looks good to me, my only comment (and one it looks like @stuartmorgan raised previously), would be that it may make sense to refactor so that instead of making the calls to getCacheDir and getSharedPreferences lazy, they are still done ahead of when they are needed but on another thread. I think either way it would make sense to land this first though and have that be a follow up, so this LGTM!

@stuartmorgan have your thoughts on this changed since that initial discussion? If not I can make an issue to track. Also will hold off on autosubmit to give you time to do a secondary review.

Would you have any pointers on how to go about calling getCacheDir() and getSharedPreferences() ahead of when they are needed? How would calls to methods such as ImageCache.saveResult() ensure that SharedPreferences have been initialized?

[stuartmorgan-g]

@stuartmorgan have your thoughts on this changed since that initial discussion? If not I can make an issue to track. Also will hold off on autosubmit to give you time to do a secondary review.

@JeroenWeener and I discussed this last week offline; the issue is that if we pre-initialize the cache on a different thread, it'll be on a thread that's not the same as the thread used by the platform channel system's background task handling, so going that route would mean either we'd have to do locking around the cache access, or we'd have to do all thread handling ourselves instead of using the background task annotation.

(To clarify my old comment, the reason making it lazy was listed as the non-preferred option there was that it was about making it lazy but not on a different thread, which would only solve part of the problem. This approach is lazy, but more importantly also on a different thread.)

[gmackall]

@stuartmorgan have your thoughts on this changed since that initial discussion? If not I can make an issue to track. Also will hold off on autosubmit to give you time to do a secondary review.

@JeroenWeener and I discussed this last week offline; the issue is that if we pre-initialize the cache on a different thread, it'll be on a thread that's not the same as the thread used by the platform channel system's background task handling, so going that route would mean either we'd have to do locking around the cache access, or we'd have to do all thread handling ourselves instead of using the background task annotation.

(To clarify my old comment, the reason making it lazy was listed as the non-preferred option there was that it was about making it lazy but not on a different thread, which would only solve part of the problem. This approach is lazy, but more importantly also on a different thread.)

Got it, with that context it sounds to me like it would add more complexity than is worth when compared to the approach in this PR, so I retract my previous comment!

[stuartmorgan-g]

LGTM!

I wish we could test the actual behavior (i.e., that no I/O is happening on the main thread), but I don't know of any way to capture that in an integration test, and since the thread management is happening in the engine we can't do it via native unit tests.

[stuartmorgan-g]

It might have been simpler to keep this class as it was, and make the ImageResizer on demand instead. But this is fine too.

[stuartmorgan-g]

The tree status here is stale; overriding.