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

fix: Ensure proper sequencing during initialization #1232

Merged
merged 1 commit into from
Nov 2, 2021

Conversation

DaveTryon
Copy link
Contributor

Details

Fix canary upgrade problem by ensuring that ordering assumptions are respected during initialization. For the autoupdate to work as expected, it's important that Container.SetAutoUpdateReleaseChannel gets called before Container.GetDefaultInstance. When #1204 was added, the code is in the proper sequence, and the upgrade works as expected when built for the debug config. When built for the release config, though, I was able to prove that the calls were occurring in the wrong order. It seems that the optimizer decided to change the sequence of calls between the API's, meaning that the autoUpdate code always loaded the production data.

The change here is to move the telemetry initialization code into a separate method so the optimizer can no longer change the call order. As an aside, enabling telemetry in a method called PopulateConfigurations was a bit off, so having it in a separate method is also better coding style. I included a comment in PopulateConfigurations as a reminder about the calling order dependency.

Motivation

Address #1226

Context

Pull request checklist

Note: After the PR has been created, certain checks will be kicked off. All of these checks must pass before a merge.

@DaveTryon DaveTryon requested a review from jalkire November 2, 2021 17:25
@DaveTryon DaveTryon requested a review from a team as a code owner November 2, 2021 17:25
Copy link
Contributor

@jalkire jalkire left a comment

Choose a reason for hiding this comment

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

It would be nice if we could have a more robust way to avoid this risk in the future, but I can't think of any great options

@DaveTryon DaveTryon merged commit ada5b18 into microsoft:main Nov 2, 2021
@DaveTryon DaveTryon deleted the fix-canary-update branch November 2, 2021 17:49
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.

2 participants