Skip to content
This repository has been archived by the owner on Nov 2, 2018. It is now read-only.

Avoid race when scopeValidation is enabled #470

Merged
merged 1 commit into from
Jan 4, 2017
Merged

Conversation

pakrym
Copy link
Contributor

@pakrym pakrym commented Dec 21, 2016


public void ValidateCallSite(Type serviceType, IServiceCallSite callSite)
{
var scoped = VisitCallSite(callSite, default(CallSiteValidatorState));
if (scoped != null)
{
_scopedServices.Add(serviceType, scoped);
_scopedServices[serviceType] = scoped;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct to update the value if the key already exists? Or would it be more correct to use TryAdd(), so the value is not updated if the key already exists? Or does it not matter and either would be equally correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are displaying only one scoped service of multiple possible so I don't think there is a difference which one exactly.

@pakrym pakrym merged commit f7fcfa7 into dev Jan 4, 2017
@davidfowl
Copy link
Member

/cc @Eilon @DamianEdwards should this go into 1.1.x?

@DamianEdwards
Copy link
Member

I don't know if there's any runway left for 1.1.1. @Eilon?

@halter73
Copy link
Member

Both a pro and con of merging this is that scopeValidation is opt-in. Presumably not many people are calling collection.BuildServiceProvider(validateScopes: true).

@davidfowl
Copy link
Member

The problem is that there's no workaround if you do want to use it. That kinda bug IMO classifies as patchable. 1.1.2 maybe?

@DamianEdwards
Copy link
Member

Yeah I think it should be considered. I think it'll have to wait for 1.1.2 though

@Eilon
Copy link
Member

Eilon commented Feb 22, 2017

Too late for the current patch, for sure. Can someone log a new bug for DI's next milestone so that we can consider? Please include some reasonable justification in terms of customer scenario.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants