chocolatey / chocolatey/choco

ISourceRunner instances are not kept between method calls to the ChocolateyPackageService

Open
#3,029 0 comments 0 reactions 0 assignees View on GitHub
0 - Triaging Bug
Dominant language
C#
Stars
11.5k
Forks
960
PR merge metrics
No merged PRs in 30d

Description

### Checklist

- [X] I have verified this is the correct repository for opening this issue.
- [X] I have verified no other issues exist related to my problem.
- [X] I have verified this is not an issue for a specific package.
- [X] I have verified this issue is not security related.

### What You Are Seeing?

Currently, calling ChocolateyPackageService methods will always result in the code being run on a _new_ instance of the ISourceRunner. For example:

https://github.com/chocolatey/choco/blob/80e76cdfdb2c956dd613ccdb179c725d986762f1/src/chocolatey/infrastructure.app/commands/ChocolateyInstallCommand.cs#L452-L456

These two commands pull in two separate ISourceRunner instances to do their work, meaning we use two distinct instances for setting up and actually running the command. This is very error-prone (see #2953) and a bit wasteful in terms of resources etc.

### What is Expected?

Calling methods on the same instance of the ChocolateyPackageService should probably call into the _same_ ISourceRunner instance(s), unless a different configuration is passed in and a different source runner needs to be used.

Part of the issue is that ISourceRunners are added to the IoC container via the `container.RegisterAll()` method in the `SimpleInjectorContainerRegistrator`, which doesn't have an overload allowing us to specify any lifetime for the services. This _might_ be resolvable in a newer version of the SimpleInjectorContainer than is being currently used on the support/1.x branch, if it provides a new overload for multi-services to specify a lifestyle to use. (ie: it may already be possible on the develop branch)

The SimpleInjector documentation also indicates that we might be able to adjust the default lifestyle on the container itself by setting certain container options (see https://simpleinjector.org/ReferenceLibrary/?topic=html/P_SimpleInjector_ContainerOptions_LifestyleSelectionBehavior.htm and https://simpleinjector.org/ReferenceLibrary/?topic=html/M_SimpleInjector_Container_RegisterCollection__1_1.htm).

Another alternative is we refactor the ChocolateyPackageService so that we make the `ensure_source_app_installed()` logic is run by that class directly before running any other action, perhaps by moving some of its code into the `perform_source_runner_action` / `perform_source_runner_function` code path and removing the separate `ensure_source_app_installed()` method. Depending on the finer details this might end up being a public API change though, and may not be desirable.

### How Did You Get This To Happen?

Was discovered while looking into the cause & fix for #2953

### System Details

N/A

### Installed Packages

```bash
N/A
```

### Output Log

```bash
N/A
```

### Additional Context

_No response_

Contributor guide

Open the contributing guide

Research direction

Start with ChocolateyInstallCommand.cs at the linked calls, then trace ChocolateyPackageService and the ISourceRunner registrations in SimpleInjectorContainerRegistrator. Review #2953 and the SimpleInjector lifetime documentation before choosing an approach; done means repeated service methods reuse the appropriate ISourceRunner instances without breaking configuration-specific behavior or the public API.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend, cli
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.