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

[5.1] Fix contextual bindings when instances and aliases are involved #15637

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
4 participants
@thecrypticace
Copy link
Contributor

commented Sep 28, 2016

Fixes #12695

Note: This issue is present on 5.1, 5.2, and 5.3. I have a branch ready to PR for just 5.3 if that's preferable.

thecrypticace added some commits Sep 28, 2016

Add tests
# Conflicts:
#	tests/Container/ContainerTest.php
@taylorotwell

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

One thing that concerns me about this is we are now spinning through every single alias for every single container resolution. I'm concerned about the performance of that considering how much the container is used.

@thecrypticace

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2016

Yeah, I definitely thought about that part of it. I haven't done any testing with Blackfire.io to see what the impact actually is. I'll do that today if I can get to it.

It might require some internal structural changes to alleviate any performance issues. The alias list the container keeps is keyed the the opposite direction for this particular use case.

My original implementation modified the contextual binding when defining it but this does not work if the alias is defined after the binding is created.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

Is this actually a bug fix, or a feature request?

@thecrypticace

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2016

bug fix.

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Sep 28, 2016

I'm not sure it is a "fix" necessarily.

@thecrypticace

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2016

ping @taylorotwell:

Did a test of the current 5.1 container versus the one in this PR:

  • 1,000,00 aliases + 1,000,000 instances + 1,000 make calls
  • 1.5s -> 63s

Yeah. 42x slower. That's not even remotely acceptable.

I'm making some changes which appear to bring that number back down to ~1.6s. Unfortunately the container must keep a second array of aliases with the same information (but flipped / regrouped). I'll keep exploring the possibilities here and update this PR when I've got things ready.

All numbers are reflective of PHP 7.0.11 on:

  • macOS 10.11.6
  • MacBook Pro (Retina, 15-inch, Mid 2015)
  • 2.8 GHz Intel Core i7
  • 16 GB of RAM
@thecrypticace

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2016

I'm not sure it is a "fix" necessarily.

I'd definitely say it is because the container is currently ignoring what you are telling it to do under a very specific situation.

$container->when(StdErr::class)->needs(LoggerInterface::class)->give(MyCustomLogger::class) works all the time (giving StdErr a MyCustomLogger) except when:

  1. There's an explicitly registered instance under LoggerInterface in the container.

This is a general proclamation of "when I ask for LoggerInterface give me X" and you can't tell the container to be any more specific for any class because contextual bindings currently fail in this situation (so one must resort to manual construction).

The container acting like this is somewhat understandable because you've registered an instance under the exact thing that was asked for by the contextual binding but it is still super unintuitive because in the end: it is ignoring what I've asked of it

Or, 2:

  • There's an alias saying LoggerInterface is actually log
  • There is a binding (not just instances in this case) registered for log

This is just such a weird edge case that it doesn't make sense for the container to completely ignore what I've asked it to do.

And just from a glance fixing item 2 without fixing item 1 would probably require a fairly large amount of refactoring.

All this being said:

  • It can be worked around.
  • I'm aware this may be a super weird edge case in using the container. I'll retarget with another PR pointed at 5.3 (or 5.4) if requested.
@taylorotwell

This comment has been minimized.

Copy link
Member

commented Sep 29, 2016

Yeah I think you're going to need a faster look-up table. Update the PR with that example so we can look at it if you have time.

@thecrypticace

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2016

Updated with an example implementation (and the CS isn't correct w/ respect to docblocks and what not). The table is not kept in-sync in all places but I'll get to doing this properly at some point today.

@thecrypticace

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2016

ping @TheoKouzelis

I confirmed the issue you saw and just fixed it on the test repo. Want to do some testing before I update this PR?

@TheoKouzelis

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

ping @thecrypticace

That has fixed my issue, thanks. Objects that depended on Psr\Log\LoggerInterface now fall back to the logger created from the main Laravel config/app.php L:123 when there is no contextual binding.

@TheoKouzelis

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2016

Here is the manual test that I did if you are interested TheoKouzelis/contextual-binding-issue@a225cb6

@thecrypticace

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2016

Excellent! PR updated. I'll be doing some more testing with the container to ensure there's no more breakage. It's a rather important piece of the framework.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Oct 5, 2016

What's the status of this?

@thecrypticace

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2016

I'm going to do a little more testing in the next day or two, update the PR to be sure both alias lists are kept in sync, and confirm whether or not it is ready.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Oct 10, 2016

Any updates?

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Oct 18, 2016

Do you still have plans of finishing this or should we try to find someone else to take a look?

@TheoKouzelis

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2016

I am happy to spend sometime on this. Does any one have any pointers on what is needed to get this over the line?

@thecrypticace

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2016

Do you still have plans of finishing this or should we try to find someone else to take a look?

Yes. Sorry. I did a bit more testing and everything looks good. Haven't had time to finish this up.

I am happy to spend sometime on this. Does any one have any pointers on what is needed to get this over the line?

The two sets of aliases need to always stay in sync. I believe that was the last thing on my list.

@TheoKouzelis

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2016

OK cool, I will have a look in the next couple of days and try to contribute with some unit tests

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

Thanks @TheoKouzelis. Have you had a chance to look into it?

@TheoKouzelis

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2016

I have had a look through tonight and I have added a test to check that the aliasAbstract property is flushed along with the aliases property thecrypticace#1 .

Apart from that I don't see any other area's the two properties could go out of sync. I thought there could be an issue with https://github.com/thecrypticace/framework/blob/patch-6-for-51/src/Illuminate/Container/Container.php#L339 but I couldn't write a which failed

@thecrypticace

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2016

I left some feedback on the PR. I cannot write a test at all which will fail when the lists are kept in sync. There is a scenario which could give rise to it but it is currently not supported and may not be feasible to ever be (well — barring a major refactor).

It's a scenario that's super unlikely to ever come up in practice anyway. (see thecrypticace#1 (comment))

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Oct 26, 2016

So is this considered ready to merge then?

@thecrypticace

This comment has been minimized.

Copy link
Contributor Author

commented Oct 26, 2016

Yes.

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

So if something needs to be contextually built can it ever be bound as an instance after this PR? For example if I want a controller method to have SomeLogger implementation of the Logger interface, and SomeLogger is bound as an instance, will that be respected or will the container create a new instance every time?

@taylorotwell

This comment has been minimized.

Copy link
Member

commented Nov 11, 2016

Closing pending inactivity. If someone wants to take this code and re-open it when they are ready that's fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.