1529611 - Glean: ensure all pings are serialized before scheduling WorkManager task

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Travis Long [:travis_]]

https://github.com/mozilla-mobile/android-components/pull/1973#discussion_r258881016

Comment 1

[Travis Long [:travis_]]

I don't think this is a problem the way that this is currently implemented.

Right now Glean.handleEvent() is called when the app goes into the background, which in turn calls assembleAndSerializePing() which assembles the ping and then calls .store() on the PingStorageEngine.

PingStorageEngine.store() is using a BufferedWriter on the same thread as the calling function and since it's calling flush() and/or close(), then all buffered writes should be completed before flush() or close() returns.

Since the PingUploadWorker isn't scheduled until after the writes are completed for all the pings, it should never be scheduled ahead of when the buffered writes have completed.

Comment 2

[Travis Long [:travis_]]

I was just considering the impact of serializing the pings on the main glean thread, but then I realized that this should only really be happening when the app is backgrounded, or in the case that we hit the event threshold that spawns a new ping. So, aside from the event ping assembly when it hits 500 records, this should only be happening once the user has consigned us the client app to the background and waiting on synchronous operation should be okay, or at least unnoticeable to the user. If it comes down to it, and we need to write async and then schedule the upload worker, we may need to chain the tasks together a little differently. Right now, the only thing I see that would force us to do it that way is performance, or if we start failing to serialize the pings because the OS is pulling the rug out from underneath our process before the write has been completed. Neither one of these are currently an issue, as far as I can tell.

Comment 3

[Travis Long [:travis_]]

After speaking with Alessio on this, I did miss the .launch in the store function that is doing this on a background IO thread. In light of that, I think it should be possible to use the WorkManager API's worker chaining to accomplish this. In which case it would look something like this:

    WorkManager.getInstance()
               .beginWith(firstWorker)
               .then(secondWorker)
               .enqueue()

Alessio suggested that we wait to ensure that WorkManager for the uploading proved itself reliable before moving forward with this implementation, so I'm tossing this bug back until we have had a chance for more data validation around that.

Comment 4

[Travis Long [:travis_]]

Attachment: GitHub Pull Request

I was able to make use of the Dispatchers.API test functionality with this PR so PingStorageEngine no longer needs to rely on a special wait function for testing purposes.