December 2017 – .NET BLOG

This post is just a quick reminder for me to put a bit more attention to conditional statements combined with a null-propagation operator. Nothing too fancy but I tend to overlook this when doing code review.
Starting from C# 6 we can simplify accessing members of an object which might be null thanks to ?. operator. For my perspective it is particularly useful within if statements, however, you have to be aware of how compiler rewrites the code under the hood otherwise, it is easy to introduce a bug. Consider following scenario

public Flight GetFirstPossibleFlight(Availability availability)
{
    if (availability.Flights?.Any() == false)
    {
       return null;
    }
            
    // more magic in here
     return availability.Flights.First();
}

public Flight GetFirstPossibleFlight(Availability availability)

{

if (availability.Flights?.Any() == false)

{

return null;

}

// more magic in here

return availability.Flights.First();

}

At a glance everything seems to be ok, we make sure that Flight collection is not accessed if it is null, however simple test shows that this method has a bug.

[Fact]
public void GetFirstPossibleFlight_ReturnsNull_WhenFlightsCollectionIsEmpty()
{
    var availability = new Availability
    {
        Flights = null
    };
            
    var subject = new AvailabilityScanner();

    var result = subject.GetFirstPossibleFlight(availability);

    result.Should().BeNull();
}

[Fact]

public void GetFirstPossibleFlight_ReturnsNull_WhenFlightsCollectionIsEmpty()

{

var availability = new Availability

{

Flights = null

};

var subject = new AvailabilityScanner();

var result = subject.GetFirstPossibleFlight(availability);

result.Should().BeNull();

}

We protected the code from NullReferenceException in if statement, however we failed on the return statement. This happens because our original code is rewritten by compiler as follows

public Flight GetFirstPossibleFlight(Availability availability)
{
    List<Flight> expr_07 = availability.Flights;
    bool flag = expr_07 != null && !expr_07.Any<Flight>();
    Flight result;
    if (flag)
    {
        result = null;
    }
    else
    {
        result = availability.Flights.First<Flight>();
    }
    return result;
}

public Flight GetFirstPossibleFlight(Availability availability)

{

List<Flight> expr_07 = availability.Flights;

bool flag = expr_07 != null && !expr_07.Any<Flight>();

Flight result;

if (flag)

{

result = null;

}

else

{

result = availability.Flights.First<Flight>();

}

return result;

}

Now it is clearly visible that we have to satisfy both of the conditions in order to return null. Fortunately, you still can use one-liner and have a bug-free code, the one thing which is needed is an additional null-coalescing operator in the if statement.

public Flight GetFirstPossibleFlight(Availability availability)
{            
    if ((availability.Flights?.Any() ?? false) == false)
    {
        return null;
    }
            
    // more magic in here
    return availability.Flights.First();
}

public Flight GetFirstPossibleFlight(Availability availability)

{

if ((availability.Flights?.Any() ?? false) == false)

{

return null;

}

// more magic in here

return availability.Flights.First();

}

Now the compiler will rewrite the code in a following way

public Flight GetFirstPossibleFlight(Availability availability)
{
    List<Flight> expr_07 = availability.Flights;
    bool flag = expr_07 == null || !expr_07.Any<Flight>();
    Flight result;
    if (flag)
    {
        result = null;
    }
    else
    {
        result = availability.Flights.First<Flight>();
    }
    return result;
}

public Flight GetFirstPossibleFlight(Availability availability)

{

List<Flight> expr_07 = availability.Flights;

bool flag = expr_07 == null || !expr_07.Any<Flight>();

Flight result;

if (flag)

{

result = null;

}

else

{

result = availability.Flights.First<Flight>();

}

return result;

}

Notice that AND operator was replaced with OR operator, resulting in code safe from NullReferenceException.
Source code for this post can be found here

Recently I’ve been trying to locate a performance issue in our application. Stress tests have shown an excessive usage of memory combined with too many external server requests. As usual in such cases, I’ve run the profiler and after a bit of searching, I’ve noticed that issue is connected with our caching mechanism. The problematic part was a factory delegate (responsible for populating cache, if the cache for given key is empty) which was called multiple times when concurrent requests were sent. That was a bit surprising to me as we basically were using a very naïve wrapper over ASP.NET Core MemoryCache.

public class CacheService : ICacheService
{
    private readonly IMemoryCache _memoryCache;

    public CacheService(IMemoryCache memoryCache)
    {
        _memoryCache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache));
    }

    public T GetOrAdd<T>(string cacheKey, Func<T> factory, DateTime absoluteExpiration)
    {
        return _memoryCache.GetOrCreate(cacheKey, entry =>
        {
            entry.AbsoluteExpiration = absoluteExpiration;
            return factory();
        });
    }
}

public class CacheService : ICacheService

{

private readonly IMemoryCache _memoryCache;

public CacheService(IMemoryCache memoryCache)

{

_memoryCache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache));

}

public T GetOrAdd<T>(string cacheKey, Func<T> factory, DateTime absoluteExpiration)

{

return _memoryCache.GetOrCreate(cacheKey, entry =>

{

entry.AbsoluteExpiration = absoluteExpiration;

return factory();

});

}

However simple test proved that this was an actual issue

[Fact]
public void GetOrAdd_CallsFactoryMethodOnce()
{
    var factoryMock = Substitute.For<Func<string>>();
    var optionsMock = Substitute.For<IOptions<MemoryCacheOptions>>();
    optionsMock.Value.Returns(callInfo => new MemoryCacheOptions());
    var memoryCache = new MemoryCache(optionsMock);

    var subject = new CacheService(memoryCache);

    var threads = Enumerable.Range(0, 10).Select(_ => new Thread(() => subject.GetOrAdd("key", factoryMock, DateTime.MaxValue))).ToList();
            
    threads.ForEach(thread => thread.Start());
    threads.ForEach(thread => thread.Join());

    factoryMock.Received(1)();
}

[Fact]

public void GetOrAdd_CallsFactoryMethodOnce()

{

var factoryMock = Substitute.For<Func<string>>();

var optionsMock = Substitute.For<IOptions<MemoryCacheOptions>>();

optionsMock.Value.Returns(callInfo => new MemoryCacheOptions());

var memoryCache = new MemoryCache(optionsMock);

var subject = new CacheService(memoryCache);

var threads = Enumerable.Range(0, 10).Select(_ => new Thread(() => subject.GetOrAdd("key", factoryMock, DateTime.MaxValue))).ToList();

threads.ForEach(thread => thread.Start());

threads.ForEach(thread => thread.Join());

factoryMock.Received(1)();

}

After doing a bit of a digging it turned out that this was by-design behavior, which is documented in the very end of official documentation . However, as this behavior wasn’t the one we were expecting, we decided to reduce amount of factory calls by applying custom logic. We had couple of ideas when it comes to implementation, including:

Exclusive lock over factory method
Lock per key
Lock per type

After discussion we decided to go with third option, mainly because we didn’t want to over-complicate the design, plus we wanted to allow concurrent factory method calls for different types of objects. Once CacheService was updated with following code

public class CacheService: ICacheService
{
    private readonly IMemoryCache _memoryCache;

    public CacheService(IMemoryCache memoryCache)
    {
        _memoryCache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache));
    }

    public T GetOrAdd<T>(string cacheKey, Func<T> factory, DateTime absoluteExpiration)
    {
        // locks get and set internally
        if (_memoryCache.TryGetValue<T>(cacheKey, out var result))
        {
            return result;
        }

        lock (TypeLock<T>.Lock)
        {
            if (_memoryCache.TryGetValue(cacheKey, out result))
            {
                return result;
            }

            result = factory();
            _memoryCache.Set(cacheKey, result, absoluteExpiration);

            return result;
         }
    }    

    private static class TypeLock<T>
    {
        public static object Lock { get; } = new object();
    }
}

public class CacheService: ICacheService

{

private readonly IMemoryCache _memoryCache;

public CacheService(IMemoryCache memoryCache)

{

_memoryCache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache));

}

public T GetOrAdd<T>(string cacheKey, Func<T> factory, DateTime absoluteExpiration)

{

// locks get and set internally

if (_memoryCache.TryGetValue<T>(cacheKey, out var result))

{

return result;

}

lock (TypeLock<T>.Lock)

{

if (_memoryCache.TryGetValue(cacheKey, out result))

{

return result;

}

result = factory();

_memoryCache.Set(cacheKey, result, absoluteExpiration);

return result;

}

private static class TypeLock<T>

{

public static object Lock { get; } = new object();

}

we stoped experiencing excessive memory usage.

Source code for this post can be found here

.NET BLOG

.NET, C# blog

Month: December 2017

Null-propagation operator and conditional statements

ASP.NET Core MemoryCache – GetOrCreate calls factory method multiple times