Refactor GDT Backgrounding #3896
Conversation
|
Relevant to #3893 but not necessarily a fix for it. |
There was a problem hiding this comment.
Generally, _runningInTheBackground should be set outside the queues here, because background tasks won't be created for each block of work that needs to be run in the background, they'll be created when the queue is emptied. The purpose is to run a background task to allow the queue to finish. This is accomplished by creating an immediate background task and appending a block to the queue to end that background task. If running in the background, a newly appended block should have a new background task created.
|
Made this a larger change, adjusting how we handle background tasks as a whole. I think this should help but would love some extra eyes on it. I believe the original assumption of requesting a background task when going into the background then cancelling when the completion handler gets called means every single time the app is backgrounded it'll execute for the full amount of time. This isn't what we want, especially if no work was being done. A lot of those lifecycle methods are empty now so there's some potential for refactor as well. |
There was a problem hiding this comment.
This looks more understandable to me, but I'll defer to @mikehaney24 for the approval.
| /** Creates a background task with the returned identifier if on a suitable platform. | ||
| * | ||
| * @param handler The handler block that is called if the background task expires. | ||
| * @return An identifier for the background task, or GDTCORBackgroundIdentifierInvalid if one | ||
| * couldn't be created. | ||
| */ | ||
| - (GDTCORBackgroundIdentifier)beginBackgroundTaskWithExpirationHandler: | ||
| (void (^__nullable)(void))handler; | ||
| (void (^__nullable)(void))handler | ||
| DEPRECATED_MSG_ATTRIBUTE("Use `beginBackgroundTaskWithName:expirationHandler:` instead."); |
There was a problem hiding this comment.
GDT does not support a public API so deprecation shouldn't be necessary.
There was a problem hiding this comment.
Should we bump the major version, or just break it? Seems like bumping the major version would be good since it's in the Public folder.
There was a problem hiding this comment.
And update the dependents to use it
This does a few things - split the tests into a specific multithreaded test and a recursive run without crashing. It also adds a notification for the upload to post how many events were uploaded. This should be a bit more reliable and helps the overall validation during the "run without crashing" test.
|
I've refactored the |
ryanwilson
changed the title
runningInBackground flags.
GoogleDataTransportCCTSupport/GDTCCTTests/Integration/GDTCCTIntegrationTest.m
Outdated
Show resolved
Hide resolved
|
Might as well update the CHANGELOGs, too |
|
@mikehaney24 @paulb777 I believe this is in a proper form to merge once reviewed. Added CHANGELOG entries, bumped the podspec versions, etc. PTAL when you get a chance! |
GoogleDataTransportCCTSupport/GDTCCTTests/Integration/GDTFLLIntegrationTest.m
Show resolved
Hide resolved
* Fix missing `runningInBackground` flags. * Attempt at fixing background issues with GDT. * Add missing deprecated implementation. * Transfer extra call to background call. * Style * Further review comments. * Fix testing. * Refactored FLLIntegrationTest. This does a few things - split the tests into a specific multithreaded test and a recursive run without crashing. It also adds a notification for the upload to post how many events were uploaded. This should be a bit more reliable and helps the overall validation during the "run without crashing" test. * Removed extra log. * Fix flag for Catalyst. * Disable currently non-functional test. * Further review comments. * Migrate CCT tests to match FLL. * Revert accidental partial change. * Clarify comment for starting the background tasks. * Review feedback, changelog additions and bug numbers.
Edited to better summarize the change:
This PR does a few things. Primarily, it refactors the backgrounding for GDT and GDTCCTSupport in an attempt to prevent unnecessary background tasks being generated, and hopefully not crash in the background any more. It also cancels active tasks once they have run out of background time.
This also refactors some tests - split the tests into a specific multithreaded test and a recursive run without crashing. It also adds a notification for the upload to post how many events were
uploaded. This should be a bit more reliable and helps the overall
validation during the "run without crashing" test.