Made in-memory persistence store thread-safe #676

Merged
merged 1 commit into from Dec 18, 2013

Conversation

Projects
None yet
3 participants
@CGijbels
Collaborator

CGijbels commented Dec 13, 2013

As mentioned in #597 there are some issues when using the default ApplicationPersistenceStore from Glimpse in an environment where requests are building up quite rapidly (especially with a maximum queue size of 25 requests).

I added a loadtest disguised as a unittest that clearly illustrates the problem. If you change the lines with lock (queueLock) to //lock (queueLock) and run the test, then you'll get one of the following exceptions:

System.InvalidOperationException
Collection was modified after the enumerator was instantiated.

Server stack trace: 
   at System.Collections.Generic.Queue`1.Enumerator.MoveNext()
   at System.Linq.Enumerable.FirstOrDefault(IEnumerable`1 source, Func`2 predicate)
   at Glimpse.Core.Framework.ApplicationPersistenceStore.GetByRequestId(Guid requestId) in ApplicationPersistenceStore.cs
System.NullReferenceException
Object reference not set to an instance of an object.

Server stack trace: 
   at Glimpse.Core.Framework.ApplicationPersistenceStore.<>c__DisplayClass1.<GetByRequestId>b__0(GlimpseRequest r) in ApplicationPersistenceStore.cs: line 77
   at System.Linq.Enumerable.FirstOrDefault(IEnumerable`1 source, Func`2 predicate)
   at Glimpse.Core.Framework.ApplicationPersistenceStore.GetByRequestId(Guid requestId) in ApplicationPersistenceStore.cs
System.ArgumentException
Source array was not long enough. Check srcIndex and length, and the array's lower bounds.

Server stack trace: 
   at System.Array.Copy(Array sourceArray, Int32 sourceIndex, Array destinationArray, Int32 destinationIndex, Int32 length, Boolean reliable)
   at System.Collections.Generic.Queue`1.SetCapacity(Int32 capacity)
   at System.Collections.Generic.Queue`1.Enqueue(T item)
   at Glimpse.Core.Framework.ApplicationPersistenceStore.Save(GlimpseRequest request) in ApplicationPersistenceStore.cs

By adding locks when accessing GlimpseRequests the exceptions above won't occur anymore. The performance impact can be neglected and it is always better than throwing around exceptions, don't you think?

@nikmd23 Any reason why I should not remove the passed in IDataStore? The queue is put inside the store with a private key and never retrieved from the store afterwards since it is kept as a private field inside the singleton instance of the ApplicationPersistenceStore (is created only once by the Factory ...). Or do you see reasons why we might end up with multiple instances of the ApplicationPersistenceStore all sharing the same queue? Because if that would be the case, then we need to wrap that queue so that the locking is shared as well and put that wrapped queue inside the store, or we lock on the queue instance instead of on the private queueLock field, but then we need to adapt the code that puts it in the store in the first place, so that that code is thread safe as well. Plenty of options to choose from, but it would make it a lot clearer afterwards

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Dec 13, 2013

Member

I wonder if we would be better switching over to a more robust implementation in general. Here is the implementation for a lock free circular ring buffer that @GrabYourPitchforks from the ASP.NET team built - https://github.com/GrabYourPitchforks/messagestore. I also know that @davidfowl has also brought it into SignalR when they where doing some of their big per optimisation work - https://github.com/SignalR/SignalR/blob/master/src/Microsoft.AspNet.SignalR.Core/Messaging/MessageStore.cs. Looking at the implication, it shouldn't be that hard to bring over and is something I've been meaning to do for a while.

Member

avanderhoorn commented Dec 13, 2013

I wonder if we would be better switching over to a more robust implementation in general. Here is the implementation for a lock free circular ring buffer that @GrabYourPitchforks from the ASP.NET team built - https://github.com/GrabYourPitchforks/messagestore. I also know that @davidfowl has also brought it into SignalR when they where doing some of their big per optimisation work - https://github.com/SignalR/SignalR/blob/master/src/Microsoft.AspNet.SignalR.Core/Messaging/MessageStore.cs. Looking at the implication, it shouldn't be that hard to bring over and is something I've been meaning to do for a while.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Dec 13, 2013

Collaborator

@avanderhoorn at first sight the circular buffer is not what we need? Since we are not reading messages from the beginning and process them one by one in the same added order, we are constantly looking inside the buffer to return stuff out of there.

Although we are currently using a queue, I do wonder if that is the appropriate data structure for our needs since we are constantly peeking inside the queue looking for possible matches based on request id or parent request ids. The queue semantics are only used to decide which ones need to be removed first when a certain threshold has been reached. Maybe another data structure (dictionary) would be more appropriate that is optimized for retrieval while keeping a queue with only the ids to decide which dictionary entry should be removed next?

Collaborator

CGijbels commented Dec 13, 2013

@avanderhoorn at first sight the circular buffer is not what we need? Since we are not reading messages from the beginning and process them one by one in the same added order, we are constantly looking inside the buffer to return stuff out of there.

Although we are currently using a queue, I do wonder if that is the appropriate data structure for our needs since we are constantly peeking inside the queue looking for possible matches based on request id or parent request ids. The queue semantics are only used to decide which ones need to be removed first when a certain threshold has been reached. Maybe another data structure (dictionary) would be more appropriate that is optimized for retrieval while keeping a queue with only the ids to decide which dictionary entry should be removed next?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Dec 13, 2013

Member

Ya, I knew this. I thinking if the data structure can safely be modified to do a lookup on any of the content?

Member

avanderhoorn commented Dec 13, 2013

Ya, I knew this. I thinking if the data structure can safely be modified to do a lookup on any of the content?

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Dec 15, 2013

SignalR actually has 2 versions of the MessageStore, one that's slightly augmented to handle lookup by a request id that may not map 1 - 1 with the store entries https://github.com/SignalR/SignalR/blob/master/src/Microsoft.AspNet.SignalR.Core/Messaging/ScaleoutStore.cs.

You might be able to still take advantage of the ring buffer but slightly tweak it to deal with looking up values from any point inside of it (depending on what you're storing of course).

SignalR actually has 2 versions of the MessageStore, one that's slightly augmented to handle lookup by a request id that may not map 1 - 1 with the store entries https://github.com/SignalR/SignalR/blob/master/src/Microsoft.AspNet.SignalR.Core/Messaging/ScaleoutStore.cs.

You might be able to still take advantage of the ring buffer but slightly tweak it to deal with looking up values from any point inside of it (depending on what you're storing of course).

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Dec 16, 2013

Member

@CGijbels What do you think? Worth looking at?

@davidfowl thanks for the comment.

Member

avanderhoorn commented Dec 16, 2013

@CGijbels What do you think? Worth looking at?

@davidfowl thanks for the comment.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Dec 16, 2013

Collaborator

@avanderhoorn to be honest, I wonder how far we need to go here. Currently we are talking about a queue/datastructure that will hold at most 25 items, so the current locking approach will have its additional overhead of using a lock but the time inside the lock is negligible, even when we allow more then 25 items, even then the time inside the lock is small in comparison with the additional memory we'll be using storing all that data.

You could use a dictionary keyed on the request id for quick look-up. And instead of adding the GlimpseRequest directly as a value, we could wrap it so that we can add child requests to it as well, which means we don't need to loop through the whole collection anymore when doing a look-up by parent request id. And once the parent gets removed, that list is removed as well, and per definition all requests in that parents list will be removed later on and can still be found on their id in the meanwhile.

But again, we are dealing here with 25 items, so I really wonder how far we need to go with this?

Collaborator

CGijbels commented Dec 16, 2013

@avanderhoorn to be honest, I wonder how far we need to go here. Currently we are talking about a queue/datastructure that will hold at most 25 items, so the current locking approach will have its additional overhead of using a lock but the time inside the lock is negligible, even when we allow more then 25 items, even then the time inside the lock is small in comparison with the additional memory we'll be using storing all that data.

You could use a dictionary keyed on the request id for quick look-up. And instead of adding the GlimpseRequest directly as a value, we could wrap it so that we can add child requests to it as well, which means we don't need to loop through the whole collection anymore when doing a look-up by parent request id. And once the parent gets removed, that list is removed as well, and per definition all requests in that parents list will be removed later on and can still be found on their id in the meanwhile.

But again, we are dealing here with 25 items, so I really wonder how far we need to go with this?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Dec 16, 2013

Member

I see what you are saying, it would only be if you wanted to take it further. For me I think we are ok with what we have. It was only if we wanted to take it to the next level.

Member

avanderhoorn commented Dec 16, 2013

I see what you are saying, it would only be if you wanted to take it further. For me I think we are ok with what we have. It was only if we wanted to take it to the next level.

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Dec 16, 2013

Collaborator

@avanderhoorn We could do that, but I wanted to make sure that issue #597 would get fixed, even though it is not the most ideal situation, at least it will be better then crashing 😉

I also wonder whether we should not provide another IPersistenceStore out of the box that does not rely on memory state, one that maybe uses a local db or so like we do for our own sample projects? And maybe we can then address at the same time my Google group comment here which is also related to another tracing bug we had with #550 because we are storing in memory which allows for modification of the data even after saving it to the store.

I think devs will also benefit from being able to look at previous requests after rebuilding there code, because they are now gone due to app pool recycles.

What do you think? Would you mind if I look more into that direction rather than further optimizing this?

Collaborator

CGijbels commented Dec 16, 2013

@avanderhoorn We could do that, but I wanted to make sure that issue #597 would get fixed, even though it is not the most ideal situation, at least it will be better then crashing 😉

I also wonder whether we should not provide another IPersistenceStore out of the box that does not rely on memory state, one that maybe uses a local db or so like we do for our own sample projects? And maybe we can then address at the same time my Google group comment here which is also related to another tracing bug we had with #550 because we are storing in memory which allows for modification of the data even after saving it to the store.

I think devs will also benefit from being able to look at previous requests after rebuilding there code, because they are now gone due to app pool recycles.

What do you think? Would you mind if I look more into that direction rather than further optimizing this?

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Dec 18, 2013

Member

It was pointed out by @nikmd23 that the same issue came up here a while back - NuGet/NuGetGallery@55a6acd#diff-0.

Member

avanderhoorn commented Dec 18, 2013

It was pointed out by @nikmd23 that the same issue came up here a while back - NuGet/NuGetGallery@55a6acd#diff-0.

avanderhoorn added a commit that referenced this pull request Dec 18, 2013

Merge pull request #676 from Glimpse/no597-make-applicationpersistenc…
…estore-thread-safe

Made ApplicationPersistenceStore thread-safe

@avanderhoorn avanderhoorn merged commit b83bf49 into master Dec 18, 2013

@ghost ghost assigned avanderhoorn Dec 18, 2013

@CGijbels

This comment has been minimized.

Show comment
Hide comment
@CGijbels

CGijbels Dec 18, 2013

Collaborator

Ah, makes sense ;-)

Luckily for them they can use the ConcurrentQueue, but we can't (yet?) because it is only available as from .NET40 and we still support NET35 (and I'm not to keen to implement a BackPort version of the ConcurrentQueue ;-))

Collaborator

CGijbels commented Dec 18, 2013

Ah, makes sense ;-)

Luckily for them they can use the ConcurrentQueue, but we can't (yet?) because it is only available as from .NET40 and we still support NET35 (and I'm not to keen to implement a BackPort version of the ConcurrentQueue ;-))

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Dec 18, 2013

Member

Thats why we didn't just pull it in back then.

Member

avanderhoorn commented Dec 18, 2013

Thats why we didn't just pull it in back then.

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Dec 18, 2013

Member

@anurse you might be interested in this PR. Its going in next release.

Member

avanderhoorn commented Dec 18, 2013

@anurse you might be interested in this PR. Its going in next release.

@nikmd23 nikmd23 deleted the no597-make-applicationpersistencestore-thread-safe branch Jul 16, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment