Fix pan domain settings refresher #41
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The actor system was instantiated for each controller. This changed when the actor system was passed down as a parameter, which in turn created more problems as the domain settings refresher was instantiated for each controller leading to a name collision. This attempts to fix it by extracting the refresher logic out of the trait, and requiring it to be injected in each controller
|
it looks good to me 👍 I just wonder if it would be cleaner if the actor was created in a method that receives an Actor System, with another one to kill it on exit for good measure? |
|
Scratch that, the sole purpose is to create that actor so no need to delay passing the system in. |
|
Cool, also passing the actor system managed by Play means when the app is turned off by play (including in dev mode) the actor system will be brought down the right way. @regiskuckaertz do you want to do a release local and test it before I merge and release? |
|
Yep, doing it right now |
|
All good 🚢 |
rtyley
added a commit
that referenced
this pull request
Sep 12, 2024
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
added a commit
that referenced
this pull request
Sep 12, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The actor system was instantiated for each controller. This changed when
the actor system was passed down as a parameter, which in turn created
more problems as the domain settings refresher was instantiated for
each controller leading to a name collision.
This attempts to fix it by extracting the refresher logic out of the
trait, and requiring it to be injected in each controller
cc @regiskuckaertz