#47 Build Element injectors with Guice Stage.PRODUCTION - #73
Open
krh372 wants to merge 2 commits into
Open
Conversation
GuiceElementLoader.load() previously called Guice.createInjector() with no explicit Stage, so every Element injector ran under Stage.DEVELOPMENT. Switched to Stage.PRODUCTION for eager singleton construction and upfront binding validation at Element-load time. Verified safe against issue #40's eager-singleton/Datastore ordering risk: GuiceSpiModule's service bindings are unscoped by default (scope is author-controlled), the repo's own @singleton usages (MongoUserDao, MongoConcurrentUtils) inject Datastore only through the LiveDatastore live-delegating proxy over the shared AtomicReference<Datastore>, and no other raw-Datastore-into-singleton pattern exists in sdk-spi-guice, sdk-guice, service-guice, mongo-guice, or sdk-mongo. Confirmed via the full sdk-test (27 tests) and sdk-spi-guice (3 tests) suites, unchanged under the new stage.
ptwohig
requested changes
Aug 26, 2026
| public Element load(final MutableElementRegistry parent) { | ||
|
|
||
| final var injector = Guice.createInjector( | ||
| Stage.PRODUCTION, |
Contributor
There was a problem hiding this comment.
I know there's more than one place we call createInjector. We should always make sure that we have a system define or environment variable to control this instead of forcing it. We should make the default the slow DEVELOPMENT mode and only turn that on in server deployments.
Reviewer feedback on the previous commit: don't hardcode Stage.PRODUCTION for Element injectors, and audit every Guice.createInjector() call site rather than treating GuiceElementLoader in isolation. Adds GuiceStages.get() (sdk-guice) which resolves the Stage from the dev.getelements.elements.guice.stage system property, falling back to the ELEMENTS_GUICE_STAGE env var, defaulting to Stage.DEVELOPMENT if neither is set or the value doesn't match a known stage. Wires it into all 8 createInjector call sites in the repo: GuiceElementLoader (per-Element injector), MongoTransactionProvider (per-transaction injector, previously also hardcoded PRODUCTION), and the top-level injectors for ElementsMain, Migrate, Setup, ApplicationNode, StatusCheck, and MavenElementsLocalBuilder (all previously implicit DEVELOPMENT with no way to opt into PRODUCTION). Net effect: default behavior everywhere reverts to pre-#47 Stage.DEVELOPMENT; PRODUCTION is now strictly opt-in for real server deployments.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
GuiceElementLoader.load()built every Element injector under Guice's defaultStage.DEVELOPMENT. Switched toStage.PRODUCTIONso@Singleton-scoped bindings are constructed eagerly at Element-load time and every binding is validated up front (fail-fast at load time instead of first use).Why this is safe
Verified against issue #40's eager-singleton/Datastore ordering risk before making this change:
GuiceSpiModule.bindAndExposeService()binds each Element service with no scope annotation — scope is entirely author-controlled, soStage.PRODUCTION's extra eagerness only affects service classes an Element author explicitly marks@Singleton..asEagerSingleton()is already the dominant pattern (194 call sites) vs. plain@Singleton(9 classes), soStage.PRODUCTIONchanges very little that isn't already eager today.@Singletonclasses that injectDatastoredirectly (MongoUserDao,MongoConcurrentUtils) are safe:Datastoreis bound asLiveDatastore, a live-delegating proxy over the mutableAtomicReference<Datastore>, not a frozen snapshot — this is the architectural fix for Mongo DAO insert throws Field-reflection IllegalArgumentException when called from a JAX-RS thread crossing an Element boundary, but not from a background thread #40's failure mode, already applied repo-wide. No other raw-Datastore-into-singleton pattern exists insdk-spi-guice,sdk-guice,service-guice,mongo-guice, orsdk-mongo.@Singletonand injecting a shared mutable dependency directly, bypassing a proxy/Provider— outside this repo's control; documented as author guidance in the accompanying manual PR (Document eager singleton construction (Stage.PRODUCTION) for Element injectors elements-manual#6).Test plan
mvn -pl sdk-spi-guice -am install -DskipTests— compiles cleanlymvn -pl sdk-spi-guice test— 3/3 tests passmvn -pl sdk-test-element,sdk-test-element-a,sdk-test-element-b,sdk-test -am install -DskipTests && mvn -pl sdk-test test— 27/27 tests pass, exercising the full isolated-classloader Element-loading pipeline (services, produced/consumed events, default/required attributes) underStage.PRODUCTION