Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix of #700 #1650

Closed

Conversation

nineninesevenfour
Copy link

Ensure the order of calls to Binder.bind(...) in combination with @ does not affect whether classes are bound explicitly or with jIT binding.

Signed-off-by: Harald Fassler harald.fassler+9974@gmail.com

@nineninesevenfour
Copy link
Author

Hello, I am a new contributor 👋, let me explain, what I did an why.

The issue popped up through https://stackoverflow.com/questions/73901723.

After some tests I found out that the issue did surprisingly not occur in combination with @ProvidedBy. After some debugging I came to the conclusion that the solution would involve delayed initialization with DelayedInitialize. I hesitated to create a distinct LinkedImplentedByBindingImpl and instead extended LinkedBindingImpl letting it implement DelayedInitialize. Also I did not change the way the binding is initialized and kept the call to ProxyFactory.notify(...), but changed it to a lambda expression. I ignored the fact, that DelayedInitialize might receive a different Injector instance. But through temporary added code (removed before committing again), I found no test that would come up with a different Injector instance here. Maybe you could guide me to find a case, where this could happen.

Unfortunately the change made InjectorImpl.cleanup() fail in context of ImplicitBindingTest.testCircularJitBindingsLeaveNoResidue(). This was because the cleanup method relied on the order of encountered dependencies of injectees. Therefor I changed encountered from a set to a map, additionally remembering if a previous encounter failed or not.

In total I tried to make the change as unobtrusive as possible.

Ensure the order of calls to Binder.bind(...) in combination with @ does not affect whether classes are bound explicitly or with jIT binding.

Signed-off-by: Harald Fassler <harald.fassler+9974@gmail.com>
@sameb
Copy link
Member

sameb commented Apr 14, 2023

Thank you for contributing this, @nineninesevenfour ! The change is in a somewhat tricky area of the code that I wrote many many years ago, so I will review it more closely next week and potentially tweak it a bit before submitting the fix.

copybara-service bot pushed a commit that referenced this pull request Apr 25, 2023
…tedBy. Instead, defer it like a normal bind(X.class).to(Y.class) does. This ensures that later bindings of Y are used.

Much thanks goes to @nineninesevenfour for their investigation (in #1650), which made fixing this much easier.

Fixes #700 and fixes #1650.

PiperOrigin-RevId: 527044205
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants