search

aliostad / CacheCow

An implementation of HTTP Caching in .NET Core and 4.5.2+ for both the client and the server
MIT License
847 stars 172 forks source link

RedisCacheStore - Buffer cannot be null #264

Closed marcinmilik closed 3 years ago

marcinmilik commented 3 years ago

We observed the issue with RedisCacheStore:

System.ArgumentNullException
Buffer cannot be null. (Parameter 'buffer')
CacheCow.Client.RedisCacheStore.RedisStore in DoGetValueAsync within CacheCow.Client.RedisCacheStore, Version=2.8.2.0, Culture=neutral, PublicKeyToken=null
Called from: System.Runtime.ExceptionServices.ExceptionDispatchInfo in Throw within System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e
CacheCow.Client.RedisCacheStore.RedisStore in GetValueAsync within CacheCow.Client.RedisCacheStore, Version=2.8.2.0, Culture=neutral, PublicKeyToken=null
Called from: System.Runtime.ExceptionServices.ExceptionDispatchInfo in Throw within System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e
CacheCow.Client.CachingHandler in SendAsync within CacheCow.Client, Version=2.8.2.0, Culture=neutral, PublicKeyToken=null
Called from: System.Runtime.ExceptionServices.ExceptionDispatchInfo in Throw within System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e

Most of the time it works well. We did a small analyze and it looks that the reason may be that entryKey expired just after the check if it exists: https://github.com/aliostad/CacheCow/blob/ed492bcb92b3582bedc043d5205dbdacf6f03ab7/src/CacheCow.Client.RedisCacheStore/RedisStore.cs#L130-L133

How about the improvement to skip KeyExistsAsync check. And only compare if result from StringGetAsync has value? Or checking if byte[] values is not null?

It looks that the similar solution was applied with success on another project. Please check that for details: https://github.com/MiniProfiler/dotnet/issues/245

aliostad commented 3 years ago

Hi, that is very weird. Essentially you are saying that between line 130 and 133 if the key expires then we have an issue and throws exception?

marcinmilik commented 3 years ago

To be more precise we think that in that case value variable contains null value and line 134 throws the provided exception: https://github.com/aliostad/CacheCow/blob/ed492bcb92b3582bedc043d5205dbdacf6f03ab7/src/CacheCow.Client.RedisCacheStore/RedisStore.cs#L134 As MemoryStream constructor does not accept null parameter. That's what we think. At least we have not found better explanation for the provided exception.

aliostad commented 3 years ago

Ah now it makes sense.

OK, that is a bug. I still believe it is a race condition which is likely but I will fix and push the next version to nuget.

Thanks for spotting it!

aliostad commented 3 years ago

This is done now. I close the issue, any problems though let me know.