7

I have singleton that fetches from DB, hence it is expensive load. It is lazy loaded.

I would like to create a method that refreshes that singleton and populates it once it is required.

the data is DB and very expensive, so I want to refresh it only once in case I have concurrent calls. (that is, if I get 500 calls to refresh, I want to restart the refresh only once)

public static PageData Instance { get { if (m_Instance == null) { lock (instanceLock) { if (m_Instance == null) { m_Instance = new PageData(); } } } return m_Instance; } } public void ReSync() { lock (instanceLock) { /* Setting to null to force the Instance to re-build */ m_Instance = null; PageData pData = Instance; } }

thanks

Improve this question
1
  • How should the system decide when a refresh is required? Is there a timespan during which all calls should be treated as the same one? Commented Mar 16, 2011 at 13:44

7 Answers 7

Reset to default
2

This is slightly incorrect, your

if (m_Instance == null)

should really be on the inside of the lock.

Sorry, didn't spot that.

Nothing is built in that can make other clients abandon calls silently if you are already refreshing. Timeouts will generate an exception I think. Perhaps maintain a stale DateTime that you can check to avoid doing the refresh on queued callers.

Improve this answer
Sign up to request clarification or add additional context in comments.

7 Comments

what do you mean? this is double check locking
m_Instance == null might be true when you check it, but false once you get to your lock. You need to lock it first to make sure you are the only one acting on the m_Instance.
this is a standard double-check-locking design!
Look again -- he checks twice (once outside, then again inside the lock).
@Adam - he is using double-check locking properly. He checks if it's null both before and after taking the lock.
|
2

In my understanding this should work.

Here is my code:

private static instanceLock = new object(); private static _refreshing = true; public static PageData Instance { get { if (_refreshing) { lock (instanceLock) { if (_refreshing) { m_Instance = new PageData(); _refreshing = false; //now allow next refreshes. } } } return m_Instance; } } public void ReSync() { if (!_refreshing) lock (instanceLock) { if (!_refreshing) { _refreshing = true; //don't allow refresh until singleton is called. } } }
Improve this answer

Comments

1

Aside from the double-checked locking being broken (correction, apparently it does work but I still find it not particularly pretty), if you have write access to m_Instance why not just set it to new PageData() there and then in ReSync?

Improve this answer

7 Comments

I prefer to use the encapsulated property init
I've seen this mentioned multiple times, and usually linking to a page such as Massif links here, that complains about Java. This post isn't about Java, though. I've seen no real evidence that double-check locking does not work in .NET. I'd be interested in seeing it, if it exists.
@Gabe, the implementation of double-checked locking is fine, it's just that the whole pattern is broken, as per the link. (further research finds that it does work now, but it's probably to be avoided: stackoverflow.com/questions/394898/…)
@mgronber: So it's not broken, it's just missing a volatile.
@Gabe: Well, it is broken if the volatile is missing. We do not know the truth, because the example does not have the definition. However, the volatile fixes only the initialization of new instance. The code is still broken because the m_Instance is read twice. It is first read to check that it is non-null and then it is read again when it is returned. A call to ReSync() may set it to null between those two reads. This bug does not even require IA64.
|
0

How about if you include a "lastRefreshed" member which you also check and lock while refreshing. So if the last refresh occurred within X time, it doesn't happen again?

Improve this answer

Comments

0

As I understand, you expect concurrent calls to Resync method? This really shouldn't happen if you call it only once in 500 requests from clients. Nevertheless, maybe then it's better idea not to just lock on instanceLock, because then singleton would be reinstantiated multiple times anyway, only not at 'same' time.

public void ReSync() { if (!m_IsResyncing) { lock (m_resyncLock) { if (!m_IsResyncing) { m_IsResyncing = true; Thread.Sleep(100); // sleep for 100ms to accumulate other locks // reinitialize here m_IsResyncing = false; } } } }
Improve this answer

Comments

0

If you want it to comply with ECMA memory model, I guess the implementation should be something like this (assuming m_Instance is not volatile):

public static PageData Instance { get { PageData instance = Thread.VolatileRead(ref m_Instance); if (instance == null) { lock (instanceLock) { instance = Thread.VolatileRead(ref m_Instance); if (instance == null) { instance = new PageData(); Thread.VolatileWrite(ref m_Instance, instance); } } } return instance; } } public void ReSync() { /* Setting to null to force the Instance to re-build */ Thread.VolatileWrite(ref m_Instance, null); PageData pData = Instance; }

If you have defined m_Instance to be volatile there is only one main difference. The m_Instance needs to be read to local variable before the null check is made, because the ReSync() method may set the shared variable to null. I also removed the lock from ReSync() as it is not really needed. The race to initialize a new instance is safe.

Improve this answer

Comments

0

I may need to understand better how the concurrancy came into this. Your description seems to fit into the scope of System.Cashing namespace. You tell that the object should store its information for an amount of time (some options are available there).

If the next request are same as the previous cashed (you define the criteria of 'identic' yourself) the caller will get the cached copy instead. In real, its means no new db connection. Just a memory request. If the valid cache criteria has passed i.e 30 minutes) the next request are against the database (for another amount of time).

Requires memory but are extremely fast and recommended in scenarios where data are reused.

Improve this answer

1 Comment

In terms of refreshing. Each individual cache can be resetted on any choosen time with a .Clear() method

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.