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

Remove the Hacks where you access Chocolatey's SimpleInjector.Container #436

Closed
ferventcoder opened this issue May 13, 2017 · 12 comments
Closed
Labels
Bug Issues where something has happened which was not expected or intended
Milestone

Comments

@ferventcoder
Copy link
Member

ferventcoder commented May 13, 2017

Is there a chance we can remove this?

public static T GetInstance<T>()
{
Func<object> instanceExp;
if (!InstanceExpCache.TryGetValue(typeof(T), out instanceExp))
{
instanceExp = GetInstanceExp<T>();
InstanceExpCache.TryAdd(typeof(T), instanceExp);
}
return (T)instanceExp();
}
private static ConstantExpression GetContainerExp()
{
return Expression.Constant(SimpleInjectorContainer.Container);
}
private static Func<object> GetInstanceExp<T>()
{
var containerType = SimpleInjectorContainer.Container.GetType();
var containerParam = GetContainerExp();
var getInstanceMethod = containerType
.GetMethods()
.Where(m => m.Name == "GetInstance")
.Select(m => new
{
Method = m,
Params = m.GetParameters(),
Args = m.GetGenericArguments()
})
.Where(x => x.Params.Length == 0
&& x.Args.Length == 1)
.Select(x => x.Method)
.First();
var block = Expression.Block(
Expression.Convert(
Expression.Call(containerParam, getInstanceMethod.MakeGenericMethod(typeof(T))),
typeof(object)));
var func = Expression.Lambda<Func<object>>(block).Compile();
return func;
}

Took me awhile to discover this one - accessing internals from something else can lead to really hard to find bugs. Here's a hint on the bug - chocolatey/choco@61cd084

This fixes calls to RegisterContainerComponents complaining that components cannot be registered after calls to GetInstance, etc.

Details

  1. Once the Container is loaded, you cannot add new things to it.
  2. ChocolateyService.GetSources() is one of the first things to run in the subprocess - note it makes a call to the Hacks file which accesses SimpleInjector.Container.
  3. Later, Lets.GetChocolatey() is called and then tries to load licensed code registration into the container, however it can't because the container has already been accessed.
  4. And then nothing works like you would expect it to.

Workaround

The fix I have for this now is to call Lets.GetChocolatey() in Program.Main() when the process is setting up so that everything gets registered properly prior to calling GetInstance.

Fix

I may not fully be aware of the reasoning here, is there a reason this is being used over setting those items up in a container specifically for the GUI?

I spent more time that I want to admit wrestling with why I could not get the Licensed code to set up properly.

Other Thoughts

I did spend quite a bit of time making the lib code a bit more robust, so at least there is that.

I'll be pushing back some PRs once I get all of this cleaned up.

@ferventcoder ferventcoder added the Bug Issues where something has happened which was not expected or intended label May 13, 2017
@ferventcoder
Copy link
Member Author

Related to #382 / #371

@RichiCoder1
Copy link
Contributor

RichiCoder1 commented May 13, 2017

@ferventcoder I literally just changed it so that GetSources now uses the background process instead haha. And the reason for that Hack is historical, but largely because there was no way do something I need to do though Chocolatey.Lib and it seemed like a pain at the time to bend the API to do it, so I just decided I'd grab the underlying service I needed directly. In this case, there was no elegant way to get sources in the way I needed (and I believe something else), so I opted to just grab the sources straight from the config file. I'd love to work on getting rid of that Hack though. I can research into it again to see why I needed it in the first place.

@RichiCoder1
Copy link
Contributor

RichiCoder1 commented May 13, 2017

Yup, I'm about 90% sure the reason for that hack is so I could get the IPackageInformationService, as there was no easy way to get pinned files and my work into creating a PR for the API became painful quickly. And then I build on that hack to get Features, Settings, and Sources as the PRs for all those became equally painful.

If we can get APIs in place to list all the above through the API, I'm pretty sure I could remove all the hacks.

@ferventcoder
Copy link
Member Author

More to the question here of what stops you fron setting up directly to IPackageInformationService and its implementation?

@ferventcoder
Copy link
Member Author

GetSources is the one I found. :)

@RichiCoder1
Copy link
Contributor

@ferventcoder What do you mean?

If I'm understanding correctly, the biggest reason I didn't just instantiate PackageInfoService directly is either it, or some important part of creating it (like an injected dependency) was internalized. And honestly, it really is a hack. I'd very much like to replace it with an "official" way.

@ferventcoder
Copy link
Member Author

None of the injected dependencies are "internalized" that I am aware of, but it could be that there is a state in something that you'd want to come from Chocolatey.

@RichiCoder1
Copy link
Contributor

That might have been it. Hence me just asking Chocolatey to create it for me.

@ferventcoder
Copy link
Member Author

Going to add a way for you to query the container directly to the api.

@RichiCoder1
Copy link
Contributor

That works too :)

@ferventcoder
Copy link
Member Author

chocolatey/choco#1294
and
chocolatey/choco#1267

Should remove most of the hacks code.

@RichiCoder1
Copy link
Contributor

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Issues where something has happened which was not expected or intended
Projects
None yet
Development

No branches or pull requests

3 participants