Fix creation of superfluous PanDomainAuthSettingsRefresher instances, make panDomainSettings a val
#155
Conversation
Having `def panDomainSettings: PanDomainAuthSettingsRefresher` in the `AuthActions` trait as a `def` means that implementors of `AuthActions` are able to make the mistake of also defining their implementation of that field as a `def`, meaning that every reference to it can potentially create _another_ instance of the refresher, as seen here: guardian/atom-workshop#361 (comment) Surprisingly, this seems to have been a problem since at least as far back as #41 in February 2018. Changing the field type to `val` forces implementers to also use `val` for that field, effectively making it a singleton, as we want. Changing the abstract field of a trait to be `val` does open up another danger due the initialization order of vals - the field could end up being evaluated as `null` if the trait immediately evaluates the field: See: https://docs.scala-lang.org/tutorials/FAQ/initialization-order.html ...consequently, I've made all `val` declarations in the `AuthActions` trait (that evaluate `panDomainSettings` in some way) into `lazy val`s. This hopefully should fix the problem.
|
@rtyley has published a preview version of this PR with release workflow run #16, based on commit 61a193c: 6.0.0-PREVIEW.fix-creation-of-superfluous-PanDomainAuthSettingsRefresher-instances.2024-09-12T1544.61a193ca Want to make another preview release?Click 'Run workflow' in the GitHub UI, specifying the fix-creation-of-superfluous-PanDomainAuthSettingsRefresher-instances branch, or use the GitHub CLI command: gh workflow run release.yml --ref fix-creation-of-superfluous-PanDomainAuthSettingsRefresher-instances Want to make a full release after this PR is merged?Click 'Run workflow' in the GitHub UI, leaving the branch as the default, or use the GitHub CLI command: gh workflow run release.yml |
…efresher instances Initially tried out in guardian/pan-domain-authentication@6e11a68, this is now separated out into its own PR, guardian/pan-domain-authentication#155.
…efresher instances Initially tried out in guardian/pan-domain-authentication@6e11a68, this is now separated out into its own PR, guardian/pan-domain-authentication#155.
| private def system: String = panDomainSettings.system | ||
| private def domain: String = panDomainSettings.domain | ||
| private lazy val system: String = panDomainSettings.system | ||
| private lazy val domain: String = panDomainSettings.domain |
There was a problem hiding this comment.
Although system & domain could have remained as defs, the truth is that they are constant val members of PanDomainAuthSettingsRefresher, so they can not change - consequently, they might as well be lazy vals (but they must be lazy, as otherwise that would immediately evaluate panDomainSettings at construction time, which we want to avoid).
| val OAuth = new OAuth(settings.oAuthSettings, system, authCallbackUrl)(ec, wsClient) | ||
| lazy val OAuth = new OAuth(settings.oAuthSettings, system, authCallbackUrl)(ec, wsClient) | ||
|
|
||
| /** | ||
| * Application name used for initialising Google API clients for directory group checking | ||
| */ | ||
| val applicationName: String = s"pan-domain-authentication-$system" | ||
| lazy val applicationName: String = s"pan-domain-authentication-$system" |
There was a problem hiding this comment.
The fields OAuth and applicationName are both made lazy because they both reference system - and to evaluate system, they need to evaluate panDomainSettings, which we want to delay until necessary, due to abstract vals in traits evaluating to null during the construction.
| val multifactorChecker: Option[Google2FAGroupChecker] = settings.google2FAGroupSettings.map { | ||
| lazy val multifactorChecker: Option[Google2FAGroupChecker] = settings.google2FAGroupSettings.map { |
There was a problem hiding this comment.
The field multifactorChecker is made lazy because it references panDomainSettings and there is again the risk that it will be null if evaluated at the point of construction.
| def panDomainSettings: PanDomainAuthSettingsRefresher | ||
| val panDomainSettings: PanDomainAuthSettingsRefresher |
There was a problem hiding this comment.
This is the core of the fix - we don't want to repeatedly evaluate panDomainSettings and get a new one each time - so it shouldn't be a def!
PanDomainAuthSettingsRefresher instancesPanDomainAuthSettingsRefresher instances, make panDomainSettings a val
This upgrades Panda from v5 to v7, allowing us to use key rotation as introduced with guardian/pan-domain-authentication#150. As Atom Workshop is pretty standard user of Panda, the upgrade is pretty simple: * Panda v6: * guardian/pan-domain-authentication#155 requires `panDomainSettings` is a `val`, not a `def`
This upgrades Panda from v5 to v7, allowing us to use key rotation as introduced with guardian/pan-domain-authentication#150. As Atom Workshop is pretty standard user of Panda, the upgrade is pretty simple: * Panda v6: * guardian/pan-domain-authentication#155 requires `panDomainSettings` is a `val`, not a `def`
This upgrades Panda from v5 to v7, allowing us to use key rotation as introduced with guardian/pan-domain-authentication#150. As Atom Workshop is pretty standard user of Panda, the upgrade is pretty simple: * Panda v6: * guardian/pan-domain-authentication#155 requires `panDomainSettings` is a `val`, not a `def`
| private def system: String = panDomainSettings.system | ||
| private def domain: String = panDomainSettings.domain | ||
| private lazy val system: String = panDomainSettings.system | ||
| private lazy val domain: String = panDomainSettings.domain | ||
| private def settings: PanDomainAuthSettings = panDomainSettings.settings |
There was a problem hiding this comment.
Why does this remain a def? settings seem to be the way to panDomainSettings in most instances…
There was a problem hiding this comment.
Really great question - there are two things that you need to know, to understand why we have def rather than val in AuthActions on def settings: PanDomainAuthSettings = panDomainSettings.settings.
-
AuthActions.panDomainSettingsis actually aPanDomainAuthSettingsRefresher- it should really be called something likepandaSettingsRefresher(tho' changing the name would sadly mean additional disruption for consumers of the Panda library, of course): -
settings- the field we're reading onPanDomainAuthSettingsRefresherwhen we callpanDomainSettings.settingsabove - is a field that can change value over time, because thePanDomainAuthSettingsRefresherwill be periodically re-fetching it (and this is essential for key-rotation, as in Support accepting multiple public keys #150):
...so, given those two things, it definitely makes sense that AuthActions.settings remains a def!
The field
panDomainSettingsin theAuthActionstrait was defined as adef, which was problematic:pan-domain-authentication/pan-domain-auth-play/src/main/scala/com/gu/pandomainauth/action/Actions.scala
Line 36 in 65e1f65
Having the field in trait as as a
defmeans that implementors ofAuthActionsare able to make the mistake of also defining their implementation of that field as adef, meaning that every read of that field will create another instance of the refresher, as seen in Atom Workshop. We can see that each EC2 instance has lots of instances ofPanDomainAuthSettingsRefresher, all logging about the config change:Surprisingly, this seems to have been a problem since at least as far back as #41 in February 2018 (which was also dealing with a problem where the refresher was instantiated too often!).
Fix: make the trait field a
valrather than adefChanging the field type to
valforces implementers to also usevalfor that field, effectively making it a singleton, which is what we want.Changing the abstract field of a trait to be
valdoes open up another danger due the initialisation order ofvals - the field could end up being evaluated asnullif the trait immediately evaluates the field during construction....consequently, I've made all
valdeclarations in theAuthActionstrait (that evaluatepanDomainSettingsin some way) intolazy vals.Testing
Deploying guardian/atom-workshop#366 at guardian/atom-workshop@b6e8a2c (using this PR at 61a193c) to CODE, we see that when we change the Panda settings file in S3, there is, as desired, only 1 instance of
PanDomainAuthSettingsRefresherper EC2 instance to log about it: