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

Make Glean init in Kotlin fully async #672

Merged
merged 4 commits into from Feb 13, 2020
Merged

Conversation

Dexterp37
Copy link
Collaborator

@Dexterp37 Dexterp37 commented Jan 23, 2020

I tested this both using Fenix and the Glean SDK sample app: this seem to work as expected. Accumulated data is correctly accounted for and any pre-init dispatched piece of information is sent.

This PR additionally fixes some weird bugs we had in our testing code that made our tests hang forever under certain circumstances. Se the indivudual commit messages.

TODO

  • Take this for a manual QA, make sure it doesn't break
  • Think a bit more what changing flushQueuedInitialTasks implies
  • Test this in Fenix

// the @MainThread decorator and the `assertOnUiThread` call.
MainScope().launch {
ProcessLifecycleOwner.get().lifecycle.addObserver(gleanLifecycleObserver)
}
Copy link
Contributor

@mdboom mdboom Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do this here, or could we do it outside of the coroutine (near the top where things like applicationContext are set?)

Copy link
Collaborator Author

@Dexterp37 Dexterp37 Jan 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure yet, I don't think there's anything preventing us from moving it there, before the glean init even starts.

The only two reasons for not doing that would be:

  • we don't want to register this if the glean init fails;
  • we really want to test this in Fenix before making a call!

Copy link
Contributor

@mdboom mdboom Jan 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough on both points. I think I was just coming at it from the angle of -- going off the main thread only to come back to it is more complicated. But, as you say, we don't really know until we try it in the context of Fenix.

Copy link
Collaborator Author

@Dexterp37 Dexterp37 Jan 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make sure to try that first once I get rid of the failing test and have a proper Fenix build for testing!

Copy link
Collaborator Author

@Dexterp37 Dexterp37 Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested that, but I think we should retain the current behaviour even just to make sure that we don't mistakenly register observers if init fails. Thoughts?

Note that this would also protect us against sudden/immediate going to background while initializing.

Copy link
Contributor

@mdboom mdboom Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point about not registering if we exit early for some other error during init. 👍 on this, then!

@Dexterp37 Dexterp37 force-pushed the faster_init branch 2 times, most recently from 588d246 to 9d7eadf Compare Feb 4, 2020
Dexterp37 added 3 commits Feb 12, 2020
Our test code relies on `withTimeout*` and `while(true)`
in a few places to wait for things to happen. However,
having a timeout is not enough: if the coroutine is busy,
even if the timeout elapses, the work will not be interrupted.
That's because cancellation in Kotlin coroutines is cooperative,
see [the docs](https://github.com/Kotlin/kotlinx.coroutines/blob/master/docs/cancellation-and-timeouts.md#cancellation-is-cooperative)
for more details.

This commit uses cooperative cancellation so that tests will
break, instead of hanging forever.
Without the context, the Glean init would throw
an exception which would "hang" the dispatchers
in the next async tests.
@Dexterp37 Dexterp37 changed the title WIP: Make Glean init in Kotlin fully async Make Glean init in Kotlin fully async Feb 12, 2020
@Dexterp37 Dexterp37 marked this pull request as ready for review Feb 12, 2020
@auto-assign auto-assign bot requested a review from badboy Feb 12, 2020
@Dexterp37 Dexterp37 requested a review from mdboom Feb 12, 2020
mdboom
mdboom approved these changes Feb 12, 2020
badboy
badboy approved these changes Feb 13, 2020
@Dexterp37 Dexterp37 merged commit eeb82ab into mozilla:master Feb 13, 2020
20 of 21 checks passed
@Dexterp37 Dexterp37 deleted the faster_init branch Feb 13, 2020
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.

None yet

3 participants