SimpleApiClient Unit Tests

[haacked]

Added unit tests of the SimpleApiClient and did some light code cleanup.

[haacked]

I removed the above block because we were accessing cache outside of the lock here. In theory, if ClearFormCache is called after we check contains (line 32) but before we execute the return (line 33), we could get an exception here because the item could be removed from the cache.

If we're concerned about performance here, we could consider switching to the ConcurrentDictionary, but I'm not sure this is a hot spot. Thoughts @shana?

[shana]

I'm almost certain I did this because Create will never get called at the same time as ClearFromCache, and the lock in Create is not to protect Create from ClearFromCache, but to have only the first instance that comes into Create trigger the creation, and keep everyone else from never even reaching the lock.

When the team explorer home page loads, there's a series of requests that get triggered to go through Create, all at the same time - one for each section and navigation item. If there's already an api client, great, return that, but if there isn't, whoever gets to the lock first gets to create it. Since the section gets created first and there's a slight delay between it and the nav items being created, that means that potentially the nav items never even hit the lock and just return immediately.

I need to confirm this, and maybe change the names on these locks to be more clear about what their role is.

[haacked]

Seems like ConcurrentDictionary would be useful for this then.

[shana]

Yeah, it might be. I'm looking at it now, and ConcurrentDictionary has this interesting remark on its MSDN page (https://msdn.microsoft.com/en-us/library/ee378677(v=vs.110).aspx):

If you call GetOrAdd simultaneously on different threads, addValueFactory may be called multiple times, but its key/value pair might not be added to the dictionary for every call.

That means that it can potentially create multiple instances of SimpleApiClient and then discard them, returning only the first one created (or the first one inserted, whichever comes, erm, first). Given that the SimpleApiClient ctor doesn't do anything, that should be fine.

[haacked]

Yeah, I'm fine with that behavior because creating them is cheap. Especially if it's discarded. Locks are less cheap.

[shana]

Yes, these were awfully silly bits of code

[haacked]

😛

[shana]

Added the ConcurrentDictionary implementation! Everything else looks ✨