COLDBOX-1420 Keep WireBox mappings registered when first metadata processing fails - #684
Merged
Merged
Conversation
…fails WireBox removed a mapping when its first mapping.process() call failed. The first caller received the original error. Later lookups failed with Injector.InstanceNotFoundException until the application was reinitialized. This turned a temporary load error into a lasting outage for explicit binder.map().to() mappings because WireBox could not recreate them. WireBox now keeps the failed mapping registered and unprocessed. The next lookup retries processing. The original caller still receives the original error. Retrying is safe because Mapping.process() marks the mapping as discovered only after processing succeeds. An exclusive lock prevents concurrent processing. The dependency injection methods also skip names that are already registered. Updated both places that removed failed mappings: - Injector.getInstance() - Binder.processMappings() Added four specs to InjectorLiveTest.cfc. The full WireBox test suite passes on Adobe ColdFusion 2023, BoxLang 1.15, and Lucee 5.4.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a WireBox failure mode where an explicit mapping could be permanently removed after its first mapping.process() attempt fails, causing subsequent lookups to incorrectly degrade into Injector.InstanceNotFoundException until reinit. The change keeps failed mappings registered so later calls can retry metadata processing and surface the original underlying error instead.
Changes:
- Stop deleting mappings on
mapping.process()exceptions inInjector.getInstance(). - Stop deleting mappings on
mapping.process()exceptions inBinder.processMappings()(still throws after iterating). - Add integration specs validating mappings remain registered and retries behave as expected (including multi-name mappings and recovery once a missing file is restored).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
system/ioc/Injector.cfc |
Removes delete-on-error around mapping.process() so failed explicit mappings remain registered for retry. |
system/ioc/config/Binder.cfc |
Keeps mappings registered during processMappings() failures while preserving error reporting behavior. |
tests/specs/ioc/InjectorLiveTest.cfc |
Adds specs covering retry behavior, recovery after missing file restored, processMappings behavior, and multi-name mappings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Description
WireBox deletes a mapping when its first
mapping.process()call fails. The first caller gets the real error, but every later caller getsInjector.InstanceNotFoundExceptionuntil the app is reinitialized. A temporary load error (deploy race, file lock, compile timeout) becomes a lasting outage for explicitbinder.map().to()mappings, because nothing re-registers them. Models found by folder scanning can recover; explicit mappings cannot.This PR removes the delete-on-error behavior in both places that had it:
Injector.getInstance()— the try/catch only deleted the mapping and rethrew, so it is now a directmapping.process()call.Binder.processMappings()— thevariables.mappings.delete( key )line is removed; it still throws after the loop.A failed mapping now stays registered and unprocessed, so the next lookup retries processing. The first caller still gets the original error.
Retrying is safe:
Mapping.process()only marks a mapping discovered at the very end, runs inside an exclusive lock, and the DI add methods (addDIConstructorArgument,addDIProperty,addDISetter) skip names that are already registered, so a retry does not double-register anything.The old behavior was also internally inconsistent: the delete removed only the looked-up name, so aliases registered during the failed processing kept pointing at the dead mapping, and a mapping registered under several names lost only one of them.
The delete dates to 2018 (
1adec53ce) with no ticket and no test covering it. The three othermapping.process()call sites (Builder x2, autowire) already keep the mapping on failure.Jira Issues
Type of change
Checklist
InjectorLiveTest.cfc: mapping kept after a failed lookup with a retry that re-throws the original error instead of InstanceNotFound; recovery once the missing file is restored;processMappings()keeps the failed mapping; multi-name mappings keep all names)