1

Closed

possible bug in Refresh() (or at least some inconsistent behavior)

description

Nope, the callback only gets fired once (with the cached data). I debugged it and I think I found the cause. In Invalidate(), it sets the CacheEntry's expiration date to Now (so it is expired), but then later when LoadInternal checks the CacheValueLoader's IsValid property, its expiration date is still in the future (assuming the cache has not expired yet based on its configuration). At that point, it calls StartLoading with force=false and passing in the CacheValueLoader, which results in the callback being fired with the cached data.
 
I tried changing the SetForRefresh() method to always call _cacheLoader.SetExpired(), but that doesnt actually update the CacheValueLoader's expiration time (instead, it sets the state to Error, and doing so resulted in some strange behavior). I then modified the CacheValueLoader.SetExpired() method to set its expiration date to Now, and that seems to work OK, though I also had to comment out the part about setting the state to Error, so I am not sure of what downstream effects it is having.
 
Since I am using this in a background agent and need to wait for the Refresh() to complete, my other work-around is to call LoadFromCache() first (which is synchronous) and then call Refresh. That works but unnecessarily slows things down since I dont want/use the cached value read from disk.
 
Anyway, I figure with the holidays that this would be something that wouldnt be fixed right away and it is not blocking me since I have a few work-arounds, but I thought I would report it so you could take a look.
 
Thanks,
 
  • brian

file attachments

Closed Jan 24, 2012 at 6:30 PM by shawnburke

comments

shawnburke wrote Jan 5, 2012 at 2:18 PM

Attached is a proposed fix, please drop those two files into your AgFx source dir and test

wrote Jan 5, 2012 at 8:23 PM

briandunnington wrote Jan 5, 2012 at 8:23 PM

No dice. I cant tell everything that was changed, but the code at the root of the issue is the same. Even though the cacheItem.SetForRefresh() is called (though it doesnt appear to use the 'live' parameter at all), later when the code runs the LoadInternal() method and gets to the following line, the _cacheLoader.IsValid is still returning true (because the cache is still valid):

var isCacheValid = _cacheLoader.IsValid;

At that point, the code just uses the cached value anyway.

I am going to attach my test case so you can test it the same way I am just to be sure I am not overlooking anything. My test case is just your Weather sample app with a few modifications to the MainPage.xaml.cs, so just copy that over your existing file and then run it. The first time you run it in a new emulator, just supply a zip code and it works fine. Exit the app, then re-run it. This time, I force a call to Refresh(), but only cached data is returned.

shawnburke wrote Jan 5, 2012 at 11:48 PM

OK - well my test case (which I should have included) must have handled a different case. Darn.
Thanks for attaching, I'll take a look.

What I saw was that the cache was being reset, which I think is the root of the wrong behavior. In a Refresh(), it should only try to fetch a new value, because the cache shouldn't have changed anyway. If it did, the Load would have returned it and/or any instances using the data would have gotten property changes and whatnot.

So that's a big of a hard part here, I tried a few fixes but then backed them out because it didn't seem like the right thing. In my thinking, that's the difference between Refresh and Invalidate. Invalidate should set everything back to it's original state (I think, I'm still torn on this) whereas Refresh, if cached data is already loaded, should ONLY do a live load, and wait for success/fail on that. It shouldn't be notifying on a cache load in any case, which is why I was addressing this at a lower level. I'm also hesitant to futz with the cache loading code much - it's really easy to have an accidental side effect.

In any case, I'll try running your scenario and see what I find, thanks!

southernsun wrote Jan 9, 2012 at 3:07 AM

Hi, I have the same problem. Attached zipfile didn't solve the issue. The problem is that SetExpired is not being called in SetForRefresh when called from a BackgroundTask (Windows Phone 7). I'm not expiring any problems when calling it straight from the app. I'm investigating the issue.

southernsun wrote Jan 9, 2012 at 3:08 AM

To clearify, i think the problem is in this code:
        if (cached && GetBoolValue(UsingCachedValueMask))
        {
            _cacheLoader.SetExpired();
        }
in SetForRefresh, but i will further investigate.

southernsun wrote Jan 9, 2012 at 3:24 AM

This 'fixed' it for me, but i'm not sure this is a proper fix because i don't know your code that well.
    public void SetForRefresh(bool cached, bool live)
    {
        _valueExpirationTime = DateTime.Now;

        if (!_liveLoader.IsValid) {
            _liveLoader.Reset();
        }

        if (cached || GetBoolValue(UsingCachedValueMask))
        {
            _cacheLoader.SetExpired();
        }
    }
and
    public T Refresh<T>(LoadContext loadContext, Action<T> completed, Action<Exception> error) where T : new() {
        var cacheItem = Get<T>(loadContext);
        if (cacheItem != null)
        {
            cacheItem.SetForRefresh(true, true);
        }
        return Load<T>(loadContext, completed, error);            
    }
I guess this is not a proper fix, but a workaround to make it work. It's kinda strange that in the original AgFx version that is now available on Nuget the Refresh works when called manually from my app, but won't work from a ScheduledAction (PeriodicTask).

I hope this helps. I'm willing to test any new fixes if you have to time to make one.

southernsun wrote Jan 10, 2012 at 2:05 PM

Just wanted to know that the proposed fix doesn't seem to be working in the live environment for some reason. During testing, when connected with Visual Studio, it worked, but tested it for 1 1/2 day on my phone as a deployed application it doesn't seem to be working.

southernsun wrote Jan 10, 2012 at 2:06 PM

with "proposed fix" i mean the fix that i proposed.

wrote Jan 12, 2012 at 10:42 PM

shawnburke wrote Jan 12, 2012 at 11:24 PM

Hi guys -

So I need a reliable repro for this. Brian, I took your repro, slapped it over the top of my main.xaml.cs file in the Weather app, and did the following things:

1) In WeatherForecastVm.cs I changed the policy to CacheThenRefresh
2) In WeatherForecaseVm, I added the following code:
    public WeatherForecastVm()
    {
        Version = _v++;
    }

    public WeatherForecastVm(string zipcode): base(new ZipCodeLoadContext(zipcode))
    {
        Version = _v++;
    }

    private static int _v = 0;

    public int Version { get; set; }
Which gives each instance a Version number on creation so I can easily tell them apart.

3) I put a breakpoint on in btnAddZipCode_Click in the success handler:

-> info.Visibility = Visibility.Visible;

I run the app and hit "go" several times. Each time I do so, the Version number of the object passed in as the "vm" parameter is incremented. So it's not the cached value, it's the new one that was created due to serialization.

I have two of you telling me this is happening, so I completely believe it, I just have yet to see a case.

Any chance we could get a UnitTest that shows this failure?

SB

southernsun wrote Jan 13, 2012 at 1:09 AM

Hi shawn, Thanks for your commitment, I will try to make an example application and post it here in the next few hours or next two days.

briandunnington wrote Jan 13, 2012 at 10:03 PM

Keep in mind that the bug only manifests itself when Refresh is called before any calls to Load (or any calls that load the data behind the scenes). Once Load has been called, Refresh seems to work properly.

When I tested it, I used my modified MainPage.xaml.cs and then did this:
  1. Open the app, enter a zip code, and let the data populate.
  2. Back out of the app
  3. Relaunch the app (which triggers the modification I made to call the button click event immediately, which does a Refresh before anything else)
At that point, you can see that the data that comes back in the 'success' handler is from the cache. I can put together a quick test case as well that is less contrived.

wrote Jan 13, 2012 at 10:15 PM

briandunnington wrote Jan 13, 2012 at 10:15 PM

Ok, here is a test case you can use that exhibits the issue. Just run the app the first time and then put in a zip code. Back out of the app and re-run it. You can put a breakpoint in the DoRefreshTestCase() method, but you can also see in the emulator that the 'last updated date' is always an old value.

wrote Jan 20, 2012 at 9:53 PM

Resolved with changeset 85589.

shawnburke wrote Jan 20, 2012 at 9:54 PM

OK, I ended up just removing the check for whether the entry was using the cache or not, so in all cases calling refresh invalidates the cache.

All the tests pass and it fixes the issue at hand via brian's repro. Let me know if you're still having problems.

briandunnington wrote Jan 21, 2012 at 5:40 PM

out of curiosity, which files/methods changed? i looked at the changeset and did a diff on CacheEntry and DataManager but didnt see any differences regarding this issue. thanks.

briandunnington wrote Jan 21, 2012 at 5:54 PM

I dont think the fix made it into changeset 85589. I cant see what has changed, and when I run the same test case using the updated code, the issue still persists. Thanks for looking into this.

shawnburke wrote Jan 22, 2012 at 5:50 AM

Grrrr...dammit, I don't know what happened. Hopefully the changes are all still on my machine at work and just didn't get picked up. I'll look Monday, sorry.

shawnburke wrote Jan 24, 2012 at 5:39 PM

** Closed by shawnburke 1/20/2012 2:53 PM

shawnburke wrote Jan 24, 2012 at 5:39 PM

Diff didn't make it into checkin.

wrote Jan 24, 2012 at 5:40 PM

wrote Jan 24, 2012 at 6:30 PM

Resolved with changeset 85706.

briandunnington wrote Jan 24, 2012 at 7:58 PM

The latest changes from changeset 85706 work for me in my test cases and in my real app. Thanks for sticking with this.

shawnburke wrote Jan 25, 2012 at 3:30 PM

Thanks for reporting back, and bigger thanks for putting up with how long it took...ugh.

southernsun wrote Jan 27, 2012 at 1:31 AM

Thanks, my test-case also works. Thanks Shawn and Brian for your work on this!!! Highly appreciated!

wrote Feb 14, 2013 at 1:53 AM

wrote May 16, 2013 at 7:17 AM