Memory leak imminent when using CacheCow.Client.CachingHandler with non-singleton ICacheStore

[slawomir-brzezinski-at-interxion]

Hi. Our code is as follows:

using (var client = new HttpClient(
                    new CachingHandler(
                        new RedisStore(...)
                    )
                    { InnerHandler = new HttpClientHandler() }
))
{
    Do something with the client.
}

(the real scenario actually has the whole construction extracted to a factory method)

This CachingHandler constructor (declared here) leaves the _disposeCacheStore flag unassigned, effectively making it false.

The RedisStore used here is something that leaks many resources if not disposed and this is exactly what happens, because this API doesn't even leave an option to dispose it.

I understand that https://github.com/aliostad/CacheCow/issues/47#issuecomment-137622213 has requested for the cache store to not be disposed by default, which caters for the cases when it's coming from an IoC container's singleton instance.

But using a case like above is also a valid use scenario, so there should at least be a constructor overload which allows to pass the ICacheStore as well as the flag to initialize the _disposeCacheStore field.

[IDisposable]

Ideally the simplest thing would be to just make the two _dispose*Store flags settable as properties thus:

public bool DisposeCacheStore { get { return _disposeCacheStore;  } set {  _disposeCacheStore = value; } }
public bool DisposeVaryStore { get { return _disposeVaryStore;  } set {  _disposeVaryStore = value; } }

Then your construction would look like

using (var client = new HttpClient(
                    new CachingHandler(
                        new RedisStore(...)
                    )
                    { InnerHandler = new HttpClientHandler(), DisposeCacheStore = true }
))
{
    Do something with the client.
}

[slawomir-brzezinski-at-travcorp]

After some thinking IMHO the decision about disposal is so important that the default, if any, should rather be true. I'd side here with the fail-fast philosophy (e.g. see Fowler). Namely, I'd rather I found out second time the code calls a component that my singleton is already disposed, which is very likely to surface during development and is pretty easily fixable, than have a memory leak, which very often surfaces as late as production, like in our case. Easy fixes to the usage of disposed object are: a wrapper around ICacheStore like you suggested here, your own subclass of CachingHandler with the default false where desired, or telling IoC container to set disposeCacheStore to false.

If error on second use is even too late, one can make things safer by not providing any default, i.e. always require a flag next to ICacheStore in the constructor. This way the error pops out before first use - during compilation.

The current behavior of silently not disposing is less safe than any of two above.

Still, if you don't want to break code for existent users (while possibly fixing memory leaks for some) please at least provide the parameter. I'd side with a constructor parameter instead of a property, because then there's more chance that someone writing construction code will actually discover it and think a little about the dangerous false value.

I might create a pull request if you decide which option you'll be going for.

[aliostad]

Hi, and sorry.

In general terms, whoever makes creates disposable objects (owning them) should dispose them. Store could be shared among multiple clients hence disposing should be done outside.

Having said that, the _disposeCacheStore is set to true by default.

I know, it is an old issue but I am releasing 2.0.0 and would like to close these. If still an issue, let me open again.

[zlamma]

Having said that, the _disposeCacheStore is set to true by default

If it's really true now, then that pretty much addresses the problem as I suggested.

Thanks!

[aliostad]

@zlamma thank you and so sorry for the massive delay. I have not been happy with Server-side caching story and now and I have fixed and ready for 2.0.0 so this time will try harder to keep on top of issues.

[zlamma]

Regarding:

In general terms, whoever makes creates disposable objects (owning them) should dispose them.

Generally, I agree this is a good rule, but this rule has a limit. When the only way a disposable resource is used is through one wrapper/decorator/composite then it it really useful to not make them leaky abstractions.

One good example is .NET StreamReader, which, according to the docs: 'calls Dispose() on the provided Stream object when StreamReader.Dispose is called.'

I don't know if other people really put their RedisStore instance to some other uses but, at least in our case, the only reason for its existence was that CachingHandler.

[aliostad]

Would be good if you can have a look at the beta 2.0.0. It now supports .NET Core too and client has not changed really so should work OOB (cannot remember any breaking change).

[slawomir-brzezinski-at-interxion]

This is certainly good news as we are gradually migrating to .NET Core, but, since this is client's commercial software, I'm afraid we will only be able to move to it once it's out of the prerelease version.

[aliostad]

release is coming soon. Preview version is already out.