CodeSOD: Tangled Up in Foo

DZ‘s tech lead is a doctor of computer science, and that doctor loves to write code. But you already know that “PhD” stands for “Piled high and deep”, and that’s true of the tech lead’s clue.

For example, in C#:

<span class="hljs-function"><span class="hljs-keyword">private</span> List<Foo> <span class="hljs-title">ExtractListForId</span>(<span class="hljs-params"><span class="hljs-built_in">string</span> id</span>)</span>
{
	List<Foo> list = <span class="hljs-keyword">new</span> List<Foo>();
	<span class="hljs-keyword">lock</span> (<span class="hljs-keyword">this</span>)
	{
		<span class="hljs-keyword">var</span> items = _foos.Where(f => f.Id == id).ToList();
		<span class="hljs-keyword">foreach</span> (<span class="hljs-keyword">var</span> item <span class="hljs-keyword">in</span> items)
		{
			list.Add(item);
		}
	}
	<span class="hljs-keyword">return</span> list;
}

The purpose of this function is to find all the elements in a list where they have a matching ID. That’s accomplished in one line: _foo.Where(f => f.Id == id). For some reason, the function goes through the extra step of iterating across the returned list and constructing a new one. There’s no real good reason for this, though it does force LINQ to be eager- by default, the Where expression won’t be evaluated until you check the results.

The lock is in there for thread safety, which hey- the enumerator returned by Where is not threadsafe, so that’s not a useless thing to do there. But it’s that lock which hints at the deeper WTF here: our PhD-having-tech-lead knows that adding threads ensures you’re using more of the CPU, and they’ve thrown threads all over the place without any real sense to it. There’s no clear data ownership of any given thread, which means everything is locked to hell and back, the whole thing frequently deadlocks, and it’s impossible to debug.

It’s taken days for DZ to get this much of a picture of what’s going on in the code, and further untangling of this multithreaded pile of spaghetti is going to take many, many more days- and much, much more of DZ’s sanity.

Source: Read More