Skip to content

Detect circular self-resolution instead of StackOverflowError#423

Open
muhammadkamel wants to merge 2 commits into
flutter-it:mainfrom
muhammadkamel:fix/detect-circular-self-resolution
Open

Detect circular self-resolution instead of StackOverflowError#423
muhammadkamel wants to merge 2 commits into
flutter-it:mainfrom
muhammadkamel:fix/detect-circular-self-resolution

Conversation

@muhammadkamel

Copy link
Copy Markdown

Summary

  • Detect when a registration's factory re-enters the same type (common footgun: registerLazySingleton<Iface>(getIt.call) / untyped tear-offs) and throw a clear StateError instead of overflowing the stack.
  • Avoid eager $stackTrace interpolation in creation-error debug logging when GetIt.noDebugOutput is set (that path can make release crash dumps look "native").
  • Add regression tests and a short README note with the safe pattern (() => getIt<Impl>()).

Motivation

In production, registering an untyped tear-off of call/get as a factory for interface T recursively calls get<T>() until StackOverflowError. Some crash reporters then drown in StackTrace.toString work, which obscured the real DI bug.

Test plan

  • dart test test/circular_self_resolution_test.dart
  • dart test (full suite)
  • Confirm message points authors at () => getIt<Impl>() / named tear-offs

Fail fast with a StateError when a factory re-enters the same registration
(e.g. registerLazySingleton<Iface>(getIt.call)), and only interpolate the
catch stack when debug output is enabled.
Clear the creation-in-progress guard when the factory returns its Future,
not when that Future completes, so overlapping always-new/cached async
resolutions are not treated as circular self-resolution.
@muhammadkamel

Copy link
Copy Markdown
Author

Follow-up: fixed a Bugbot finding where overlapping getAsync on the same async factory was falsely treated as circular dependency. The creation guard now only covers the synchronous factory start (lazy async still holds until the pending creation finishes). Added regression tests.

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.

1 participant