[CSHARP-975] Move Caching of Connection Pools out of MongoServer. Created: 22/May/14  Updated: 02/Apr/15  Resolved: 19/Aug/14

Status: Closed
Project: C# Driver
Component/s: API
Affects Version/s: None
Fix Version/s: 2.0

Type: Improvement Priority: Major - P3
Reporter: Craig Wilson Assignee: Robert Stam
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Duplicate
is duplicated by CSHARP-663 Move life cycle managament of server ... Closed
Epic Link: New Client API

 Description   

Currently, MongoServer caches connection pools. As MongoServer is continued Legacy API, we need to move caching elsewhere.



 Comments   
Comment by Robert Stam [ 04/Sep/14 ]

Remember that if GetOrAdd is called by multiple threads the valueFactory may be called more than once.

When this happens, only one of the values will get added to the concurrent dictionary, and the extra ones get garbage collected. This is really only viable when it doesn't matter if a few extra redundant copies of the value get created.

With a resource like a Cluster we want to make sure that only one Cluster is created, so a simple lock statement achieves our goals better here.

Comment by Chad Kreimendahl [ 04/Sep/14 ]

I suppose you could just go straight at ConcurrentDictionary in your case. I checked our end and we always use ConcurrentDictionary in these cases, instead of the double lock check. Our double lock checking was only for some other thread-safe caching we're doing.

Jon Skeet's answer on the question suggests either keep it exactly as is or let ConcurrentDictionary do it all for you and have it be even simpler: http://stackoverflow.com/questions/6018768/double-checked-locking-on-dictionary-containskey

Very Simple Version

private readonly _registry = ConcurrentDictionary<ClusterKey, ICluster>()
 
private ICluster GetOrCreateCluster(ClusterKey clusterKey)
{
    return _registry.GetOrAdd(clusterKey, () => CreateCluster(clusterKey));
}

Comment by Chad Kreimendahl [ 04/Sep/14 ]

We certainly only use the concurrent variety of collections, when concurrency is even possibly threaded. However, a quick look at some examples of stuff that use mongo seems to show that on most, each web request is likely to create a new client. However, I highly doubt most would be doing enough requests to make it matter, except in the case of APIs.

Either way, we typically use this methodology on all locks with significant performance gains.

We'd be happy to do a comparison test once v2 is done. Simplicity is nice, too.

Comment by Robert Stam [ 04/Sep/14 ]

Thanks for the comment. I've been thinking about it, and I don't think the proposed alternative is thread safe. A Dictionary can only be safely shared among multiple reader threads as long as no other thread is modifying the dictionary at the same time. That means that even reads have to be protected.

Switching to a ConcurrentDictionary is certainly a possibility, but keep in mind that this lock is not a likely candidate for contention, so keeping things as dead simple as possible also has merit. This lock would only come into play when a new MongoClient is instantiated, and many applications create a single MongoClient and use it for the lifetime of the application. Even if the application created many new MongoClient instances, this lock would only be held just long enough to lookup the cluster instance in the registry dictionary, which would be very fast, and has almost no chance of resulting in lock contention.

Comment by Chad Kreimendahl [ 03/Sep/14 ]

Just a quick performance pattern note on a lock we saw:

Old

            lock (_lock)
            {
                ICluster cluster;
                if (!_registry.TryGetValue(clusterKey, out cluster))
                {
                    cluster = CreateCluster(clusterKey);
                    _registry.Add(clusterKey, cluster);
                }
                return cluster;
            }

If you can safely assume that the Get will at least occasionally return the cluster value, the following would be substantially faster and avoid an unnecessary lock [possibly more important than speed]. You could also consider using a ConcurrentDictionary, which is threadsafe.

New

            ICluster cluster;
            if (!_registry.TryGetValue(clusterKey, out cluster))
            {
                lock(_lock)
                {
                     // check again in case a multi-threaded app has been the first to set it in previous locks
                    if (!_registry.TryGetValue(clusterKey, out cluster))
                    {
                        cluster = CreateCluster(clusterKey);
                        _registry.Add(clusterKey, cluster);
                    }
                }
            }
           return cluster;

Comment by Githook User [ 19/Aug/14 ]

Author:

{u'name': u'rstam', u'email': u'robert@10gen.com'}

Message: CSHARP-975: Added tests.
Branch: master
https://github.com/mongodb/mongo-csharp-driver/commit/1a2ea42b4a5d30a122911d100d60186d53e241b5

Comment by Githook User [ 19/Aug/14 ]

Author:

{u'name': u'rstam', u'email': u'robert@10gen.com'}

Message: CSHARP-975: Move caching of connection pools out of MongoServer.
Branch: master
https://github.com/mongodb/mongo-csharp-driver/commit/eed6cfadf0d65e9f36f4157922583d7b1335cad4

Comment by Craig Wilson [ 03/Jul/14 ]

Where is lower than MongoClient? I mean, there has to be a hook somewhere to say, GetOrCreateCluster. I'd imagine that place exists probably in a ClusterCache, but that cluster cache is likely invoked from MongoClient. Regardless, the point of this is that it needs to be pulled out of MongoServer. I'll update the ticket title.

Comment by Robert Stam [ 03/Jul/14 ]

We probably want to cache clusters (and therefore indirectly one or more connection pools) at a lower level than MongoClient.

Currently MongoClient instances with different settings result in multiple connection pools being created, even if the settings differ only in ways that have nothing to do with the connections (e.g. ReadPreference).

There is work ongoing in CSHARP-875 related to this as well.

Generated at Wed Feb 07 21:38:20 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.