I am working on a Windows service in Visual Basic .NET. I have a bunch of data that is cached and updated by a module, and I have a small set of classes that retrieve data from that module.

When the caching module updates the data, it invokes a method on its consumers saying that says hey, dummies, dump whatever you've got that depended on the old data and update with this new hotness.

Instances of the consuming classes may be in use at the time the cache update happens, so I used SyncLock to set up critical sections in the methods that act on the data and the methods that update it. So I know that I won't be sending out expired data while an update is going on.

Okay, still with me on this?

So a pattern has developed. A lot of this cached data is in Dictionary fields on the consumers. I have a Try/Catch block that tries to look up a dictionary value and either returns the value or returns Nothing in the case of a KeyNotFoundException. Rather than writing a bunch of boilerplate, I added the following on the consuming classes' base class:

    Private Shared Function GetItem(Of TKey, TValue)(ByRef Dict As Dictionary(Of TKey, TValue), ByVal Key As TKey, ByRef Lock As Object) As TValue
        Dim Value As TValue

        SyncLock Lock
            Try
                Value = Dict(Key)
            Catch ex As KeyNotFoundException
                Value = Nothing
            End Try
        End SyncLock

        Return Value
    End Function

    Protected Function GetFruitItem(Of TKey, TValue)(ByRef FruitDict As Dictionary(Of TKey, TValue), ByVal Key As TKey) As TValue
        Return GetItem(FruitDict, Key, FruitLock)
    End Function

    Private _AppleData As Dictionary(Of AppleType, AppleCultivar)

    Public Function AppleData(ByVal [Type] As AppleType) As AppleCultivar
        Return GetFruitItem(_AppleData, AppleType)
    End Function

8/30/2018 5:19:14 PM

I think that's mostly fine in concept.

One piece of feedback I have is to avoid using try catch blocks like that, especially if you aren't disabling stack tracing.

It would be way better if you instead used some type of if or inline if with .ContainsKey, which would avoid the investment in unwinding the stack every time you trap that exception.

8/31/2018 10:52:37 AM

I hear you, but for this particular use case, a key not being present in the dictionary truly is exceptional. TryGetValue is a convenience method that checks for the key like you're suggesting, and I'd use that if it were more likely to happen. I can afford a few millseconds for the Try/Catch on the off chance an exception is thrown versus presumably more time spent checking for the key first in this case.

8/31/2018 10:58:50 AM

The fact that you have several methods which all take a Dictionary(TKey, TValue) indicates that you should factor this out into another class, instead of putting it on a consumer's base class. Where you instantiate this new class will depend upon how the Dictionaries are populated. If they are coming from the caching module, then the caching module should provide them to the consumers. If the consumers are maintaining the data in the dictionaries, then they should instantiate the class. Either way, the logic should be pulled out of the consumers and into a reusable class whose sole job is to perform the synchronization for you.

Create a wrapper class which exposes, at a minimum, a "GetItem" method with the appropriate logic built in. If the Dictionaries are maintained by the caching module, the wrapper constructor should take your original Dictionary instances and the object for synchronization. If the consumers are maintaining the Dictionaries, I would hide the Dictionaries INSIDE the wrapper class, and add a corresponding "AddOrUpdateItem" method. If the consumers are maintaining the Dictionaries, as well as using a pretty good range of Dictionary's methods, then the wrapper class should just implement IDictionary itself, using a Dictionary for its internal data storage, taking the sync object into its constructor and using it in each method as needed.

Also, I'd say you are off on the TryGetValue vs Try/Catch from a performance and practicality perspective. A Dictionary necessarily has to see if the key exists in order to retrieve it. Browsing the source code, Dictionary's implementations of TryGetValue and the indexer are identical, except that TryGetValue returns FALSE while the indexer throws an exception. You simply shouldn't use exceptions as control flow without a damned good reason.

9/10/2018 8:45:51 PM

Ah, let me clarify. My original post wasn't as clear as I thought.

aaronburro said:

Quote :

"The fact that you have several methods which all take a Dictionary(TKey, TValue) indicates that you should factor this out into another class, instead of putting it on a consumer's base class."

It's on the base class because the lock objects are shared among all instances. The first method is the generic locking accessor which is private to the base class, and the second is the protected method exposed to its subclasses. There is one method like this for each shared lock object.

I don't think it makes sense in this case to add another class that would really just be exposing an implementation detail about how the subclasses synchronize. If I could make it more generic, then maybe, but I might not even care about synchronizing access to the dictionaries in the next version. There might not even be dictionaries in the next version.

If I did add a class for that, I'd just have to provide the shared lock object to the class anyway, and that seems messy to me. I'd be maintaining state in the consumers for synchronization still, and the consumers' base class would have a shared reference to the new synchronizing class. I don't really want to add a whole separate class that tightly coupled in this case.

Quote :

"Also, I'd say you are off on the TryGetValue vs Try/Catch from a performance and practicality perspective. A Dictionary necessarily has to see if the key exists in order to retrieve it. Browsing the source code, Dictionary's implementations of TryGetValue and the indexer are identical, except that TryGetValue returns FALSE while the indexer throws an exception. You simply shouldn't use exceptions as control flow without a damned good reason."

A key not being present in this case is exceptional. The .NET runtime provides performance counters for exceptions thrown. I want to track that, and I don't want to add irrelevant code to do it via TryGetValue instead. If there's a performance concern, I'll measure the impact when it comes up and consider a change, but bending over backwards to avoid handling an exception seems premature. It's not like I'm taking a vastly different code path that makes it harder to understand what state the object is in. I'm explicitly assigning null to the return value. It's like 10 lines of code. I don't think there's value in being dogmatic here.

Thanks for reviewing, though. I feel better on the whole about it.

[Edited on September 11, 2018 at 7:41 AM. Reason : ///]

9/11/2018 7:41:02 AM

Oh so I wound up deleting every bit of this code the other day. Thanks!

9/19/2018 10:23:25 AM

Another .NET service, another "Is this sane?" moment.

I need to log a high volume of relatively small records to a SQL Server database via SOAP request. I am writing a WCF service to be hosted inside a managed Windows service.

I want the logging method to return immediately rather than make the caller wait for a response while the INSERT runs. This is important for keeping the caller running smoothly. So I decided since I have a Windows service hosting WCF, I might as well take advantage.

I moved the actual logging to a separate method on the service and created a ConcurrentQueue for records to log that's shared between the WCF service and the Windows service. When the WCF service receives a log request, it enqueues the record to be logged and returns immediately. The Windows service has a timer that periodically checks whether the queue is empty and logs records.

I like it 'cause it has the side effect of letting me smooth database writes over time, and I could suspend writes entirely with a service control method without affecting the WCF service or its callers. I could even trigger the service to switch to an alternative data store with a control method. It seems like a good idea for increasing this service's reliability.

But is it sane? I realize I need to handle draining the queue when the service receives the stop signal and whatnot, but I'm not too concerned about that.

2/5/2019 7:37:12 AM

A queue processed by another thread is a natural solution to something like this, so yes, it's sane. Logging should be as lightweight as possible, with as small a possible fuck-up surface area as you can get. At most, you would want to know that the logging request was picked up, nothing more.

The only issue you might run into is the locking overhead for the ConcurrentQueue. You said it's a high-volume request, so there could be issues. That data structure is generally good, though. We ended up writing a lock-free version of it, but we had need for it.

Also, not sure why there are two services for this.

[Edited on February 5, 2019 at 3:11 PM. Reason : ]

2/5/2019 2:53:18 PM

It's a WCF service hosted inside a Windows service. There's only one service service. The hosts it will be deployed to will not have IIS.

2/5/2019 3:19:57 PM

K, that makes sense, then. When you write the queue-processing algorithm, I would suggest grabbing all items in the queue at once (clearing the queue), processing them, and then check the queue again. You will greatly reduce contention that way. Batching the INSERTs or doing them one-by-one should be performance tested. I wouldn't know which way would work better, as I suspect it would be usage dependent.

Also, you can poll the queue on a timer, but it's better to just have a flag that indicates if the queue is being processed. No wait time for logging that way, and you can greatly simplify the algorithm that way.

2/5/2019 3:36:00 PM