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

CacheTagHelper should not cache inner contents execution result when Exception occured #5988

Closed
twirpx opened this issue Mar 19, 2017 · 4 comments
Assignees

Comments

@twirpx
Copy link
Contributor

twirpx commented Mar 19, 2017

At controller we set flag to throw exception only during first call of the action.

public class HomeController : Controller {
        private static int m_ThrowCounter;
        [ HttpGet ]
        [ Route("") ]
        public IActionResult CachedExceptionTest() {
            CachedExceptionTestModel model = new CachedExceptionTestModel();
            model.ThrowException = Interlocked.Increment(ref m_ThrowCounter) == 1;
            return View("Index", model);
        }
}

At view we throw exception when the flag set:

@model CachedExceptionTestModel
<cache expires-after="@TimeSpan.FromHours(1.0)">
    @if (Model.ThrowException) {
        throw new Exception();
    } else {
        @:OK
    }
</cache>

I suppose that CacheTagHelper should not cache exception occurs during first time call and just discard result. Second and following calls should not use cached exception, tag helper should execute inner contents and cache it only if it is not exception throwing.

In mine project for example this issue leads to the fact that when DB TimeoutException occurs during processing cached part of the page there no chance to see normal results in second later when DB load lowers. If exception occures in some page, all subsequent request to the page causes exception while cache tag not expire.

Test project attached:
CachedExceptionTest.zip

@twirpx
Copy link
Contributor Author

twirpx commented Mar 19, 2017

UPD:
It seems that second call exceptions is not cached exception but NullReferenceException at this line:
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs#L133
because result is not set in case of exception in this line:
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.TagHelpers/CacheTagHelper.cs#L107

@Eilon
Copy link
Member

Eilon commented Mar 21, 2017

@sebastienros thoughts on this? It does seem like we're inadvertently caching stuff when an exception was thrown.

@sebastienros
Copy link
Member

I have an updated unit test that is refactored, I'd like to commit on your branch but it seems you prevented that. There is a checkbox for it: https://github.com/blog/2247-improving-collaboration-with-forks

You would still be the author of the contribution.

@twirpx
Copy link
Contributor Author

twirpx commented Mar 25, 2017

@sebastienros This option was already checked. I have added you to collaborators list of forked repo, so you can push your commit there. I think this should work.

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

No branches or pull requests

3 participants