Add to Visited if in Cache #69
Conversation
| if (!isError && !context.Visited.ContainsKey(key)) | ||
| { | ||
| context.Visited.Add(key, verificationResult); | ||
| } |
There was a problem hiding this comment.
I don't think this condition is correct. We should always add the verification result here.
We already know that context.Visited.ContainsKey(key) returns false (Line 97).
We also want to maintain the postcondition that everything we look at is in context.Visited, so it should go in there regardless of whether it's an error.
|
@rynowak Updated |
| if (context.Cache.TryGetValue(key, out cacheResult)) | ||
| { | ||
| context.Visited.Add(key, verificationResult); |
There was a problem hiding this comment.
I think we also need to copy the related properties from catchResult -> verificationResult
There was a problem hiding this comment.
Line 69 also need to change to a 'TryAdd`.
Basically we need to deal with the fact that we could find out from this method that our proxy type as already been created.
|
@rynowak Updated |
| if (context.Cache.TryGetValue(key, out cacheResult)) | ||
| { | ||
| verificationResult.Constructor = cacheResult.Constructor; | ||
| verificationResult.Type = cacheResult.Type; | ||
| context.Visited.Add(key, verificationResult); |
There was a problem hiding this comment.
Add a blank line after this, it's bad Feng Shui to put a comment directly between two lines of code.
| if (context.Cache.TryGetValue(key, out cacheResult)) | ||
| { | ||
| verificationResult.Constructor = cacheResult.Constructor; |
There was a problem hiding this comment.
Add a comment here, this should explain how/why this can happen.
| var key = new Tuple<Type, Type>(sourceType, targetType); | ||
| var cache = new ProxyTypeCache(); | ||
| cache[key] = ProxyTypeCacheResult.FromType(key, sourceType, sourceType.GetConstructor(Array.Empty<Type>())); | ||
| var context = new ProxyTypeEmitter.ProxyBuilderContext(cache, targetType, sourceType); |
| public void GetProxyType_IfAlreadyInCache_AlsoAddedToVisited_FromType() | ||
| { | ||
| // Arrange | ||
| var sourceType = typeof(ProxyPerson); |
There was a problem hiding this comment.
Can you pick an example that should at least by possible?
|
@rynowak Updated |
| if (context.Cache.TryGetValue(key, out cacheResult)) | ||
| { | ||
| // If we have a key in the cache because of an operation that completed before, | ||
| // but context.Visited.ContainsKey(key) above returned false, then we must add the key | ||
| // to the context.Visited list. |
There was a problem hiding this comment.
Comments should say why not what
Addresses possible race condition in aspnet/Mvc#6292 (cherry picked from commit f035508)
Attempt to address aspnet/Mvc#6292
cc @rynowak