Skip to content
New issue

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

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

Already on GitHub? Sign in 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
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.

2 participants