4

Consider these lines of code:

ConcurrentDictionary<string, object> error = new ConcurrentDictionary<string, object>();
error.Add("hello", "world"); //actually throws a compiler error

and

IDictionary<string, object> noError = new ConcurrentDictionary<string, object>();
noError.Add("hello", "world");

I eventually figure out that all you have to do is change the IL to make the Add function private.

Now in the spirit of decoupled code I'd most likely use the Interface but it seems that Concurrent dictionary isn't too found of the Add method.

Is it safe to really use Add(I can't view the IL so I don't know if it's really thread safe.)? Or should I use the concrete type of ConcurrentDictionary<TKey, TValue> and explicitly use TryAdd.

dbarnes
  • 1,803
  • 3
  • 17
  • 31
  • 2
    Your title and actual question don't really match. [There is no such thing as a non-public member of an interface](http://stackoverflow.com/questions/516148/why-cant-i-have-protected-interface-members). – Erik Philips Apr 14 '15 at 21:27
  • @ErikPhilips not sure how the link is relevant. It's clear IDictionary<,> exposes Add, but Concurrent Dictionary doesn't which means it does Implement the interface but the function is still hidden. – dbarnes Apr 14 '15 at 21:32
  • It's not hidden. You need to understand the diference between an [Implicit Implementation versus an Explicit Implementation](http://stackoverflow.com/questions/143405/c-sharp-interfaces-implicit-implementation-versus-explicit-implementation). In either case, `Add()` is implemented and exposed. – Erik Philips Apr 14 '15 at 21:36
  • @evanmcdonnal, but it does exist and it's [documented by microsoft](https://msdn.microsoft.com/en-us/library/dd287165(v=vs.110).aspx). Did you even look? – Erik Philips Apr 14 '15 at 21:39
  • @ErikPhilips, you are correct I was misunderstanding the difference. I always thought that was just for readability sake not an actual meaning. – dbarnes Apr 14 '15 at 21:43
  • So the real difference is that if someone wrote code that you have NO idea what it does but it requires an `ICollection<>`. Now you have threads that may modify the collection in the timeframe you passed your collection, you'd probably want it thread safe, thats why `ConcurrentDictionary` implements the feature. (Real case scenario, not the defacto reason). – Erik Philips Apr 14 '15 at 21:47
  • @ErikPhilips It completely makes sense from a Code design specific case, you more or less opened my eyes to explicit vs implicit. I actually thought they went in and changed the IL manually. – dbarnes Apr 14 '15 at 21:49
  • @evanmcdonnal my guess is that they use sandcastle and it doesn't look for Explicit definitions, but I think that answer is outside the scope of this question. – dbarnes Apr 14 '15 at 21:51
  • 2
    @evanmcdonnal it's is listed in the href you gave LOL, it's under `Explicit Interface Implementations`. You funny. – Erik Philips Apr 14 '15 at 21:53
  • You may also want to look up `Program to an interface, not an implementation`. It's a basic [design pattern](http://en.wikipedia.org/wiki/Design_Patterns) of object oriented programming (very fundamental). – Erik Philips Apr 14 '15 at 22:03

2 Answers2

8

Yes, it's safe.

Have a look at the reference source for ConcurrentDictionary. The method IDictionary<TKey, TValue>.Add simply calls TryAdd and throws an exception if the key already exists.

The hiding of members of an interface is not something that requires IL modifications to be done. It can be done through explicit interface implementation in C#. This is done by leaving off the access modifier of the method and prefixing the method name with the interface name:

void IDictionary<TKey,TValue>.Add(TKey key, TValue value) {}

There are various reasons for doing this, maybe you don't want to clutter the concrete interface, or you want consumers of your class to be explicit about what method they are using if the name of the methods on the interface aren't specific enough. Also, it allows you to provide separate implementations for methods on different interfaces with the same signature (not really an issue for ConcurrentDictionary I think, but the capability is there if you need it in your own classes).

Erik
  • 5,355
  • 25
  • 39
  • 2
    I would guess it's so that you can say to someone, "if you know you're using a `ConcurrentDictionary`, you should use TryAdd" but still have it behave as an `IDictionary` when accessed through the interface. – Erik Apr 14 '15 at 21:33
  • 2
    So people will understand that if you use the concrete version, you're code should be readable and use `TryAdd`, specifically stating what your code does. – Erik Philips Apr 14 '15 at 21:33
  • I think this discussion is related... http://stackoverflow.com/questions/143405/c-sharp-interfaces-implicit-implementation-versus-explicit-implementation – Mick Apr 15 '15 at 01:55
5

Hmya, you are playing a dangerous game. The public interface of the ConcurrentDictionary class provides thread-safe methods that you can feel good about calling, knowing that they behave well.

But the Add() method is not such a method, only TryAdd() is. You get the compile error because Add() is not public, Microsoft intentionally made the method inaccessible by writing an explicit interface implementation version of it. Otherwise something they had to do, ConcurrentDictionary implements IDictionary. Whether they should have implemented that interface is debatable. But water over the bridge, they did.

And sure, you can very easily cast to access Add(). But now that feel good feeling is starting to develop frays around the edges. Quite appropriately, the only way they could implement Add() is to make it throw an exception when TryAdd() fails. Kaboom with an ArgumentException, good luck debugging that.

Are you absolutely 100 percent sure that you can deal with an exception on a worker thread when such a basic operation fails? Did you, instead of thinking about hacking IL, think about what you need to do in your catch clause? And if you did, exactly how it is that different from the code you write when TryAdd() returns false?

It is not different.

Hans Passant
  • 922,412
  • 146
  • 1,693
  • 2,536
  • This has not made it in production code, more just tinkering. I noticed this case and decided to just use the concrete type for the exact reasons you specified. I just found it a bit odd and figured it was worth a question. – dbarnes Apr 14 '15 at 22:00
  • Hmm, a nasty undebuggable exception upvoted as a feature. What are we doing here folks? Keeping fingers crossed that it won't happen? – Hans Passant Apr 14 '15 at 22:07