Device driver thread safety requirements/assumptions

[diggit]

While working on #1017 (generally RigolOscilloscope), I have noticed mutex locking in several places. So I guess some thread safety is required.

From reading of LeCroyOscilloscope I've got a feeling all runtime member data which are not constant since object creation (like m_transport) should have exclusive access.
But there are places where for example m_channelsEnabled access is exclusive

scopehal/scopehal/LeCroyOscilloscope.cpp

Lines 1166 to 1207 in 80fdea6

	lock_guard<recursive_mutex> lock2(m_cacheMutex);
	if(m_channelsEnabled.find(i) != m_channelsEnabled.end())
	return m_channelsEnabled[i];
	}
	//Analog
	if(i < m_analogChannelCount)
	{
	//See if the channel is enabled, hide it if not
	string cmd = GetOscilloscopeChannel(i)->GetHwname() + ":TRACE?";
	auto reply = m_transport->SendCommandQueuedWithReply(cmd);
	lock_guard<recursive_mutex> lock2(m_cacheMutex);
	if(reply.find("OFF") == 0) //may have a trailing newline, ignore that
	m_channelsEnabled[i] = false;
	else
	m_channelsEnabled[i] = true;
	}
	//Digital
	else if( (i >= m_digitalChannelBase) && (i < (m_digitalChannelBase + m_digitalChannelCount) ) )
	{
	//If the digital channel *group* is off, don't show anything
	auto reply = Trim(m_transport->SendCommandQueuedWithReply("VBS? 'return = app.LogicAnalyzer.Digital1.UseGrid'"));
	if(reply == "NotOnGrid")
	{
	lock_guard<recursive_mutex> lock2(m_cacheMutex);
	m_channelsEnabled[i] = false;
	return false;
	}
	//See if the channel is on
	//Note that GetHwname() returns Dn, as used by triggers, not Digitaln, as used here
	size_t nchan = i - m_digitalChannelBase;
	reply = Trim(m_transport->SendCommandQueuedWithReply(
	string("VBS? 'return = app.LogicAnalyzer.Digital1.Digital") + to_string(nchan) + "'"));
	lock_guard<recursive_mutex> lock2(m_cacheMutex);
	if(reply == "0")
	m_channelsEnabled[i] = false;
	else
	m_channelsEnabled[i] = true;

, but not in other

scopehal/scopehal/LeCroyOscilloscope.cpp

Lines 1212 to 1214 in 80fdea6

	return m_channelsEnabled[i];
	}

(fame fcn though).

Description of mutex itself does not say much

scopehal/scopehal/SCPIOscilloscope.h

Lines 49 to 50 in 80fdea6

	//Mutexing for thread safety
	std::recursive_mutex m_cacheMutex;

C++ concurrent access is tricky thing due to the C++ memory model. Today compilers are quite careful so the code often works, even though it does not follow the rules.

[azonenberg]

That actually does look like a bug in the LeCroy driver.

As a general rule, the cache mutex should be locked any time we're indexing a std::map with cached state in it. We get away with it most/all of the time because IsChannelEnabled locks the cache mutex earlier, and after the first few times the function is called during instrument startup the cache is hot and we will take the early-out path in line 1168. We might not even have any other threads running the first few times it's called so it might technically be safe in ngscopeclient, but adding a lock before 1213 is certainly the right call. I'll do that on my end.

[diggit]

This issue was more of a question and not a bug report. I just took Lecroy driver as a reference, because you are it's main developer and I expect it is the most polished one. Anyway, I think I've got the answer and bug got squished, so I guess it's a win-win :D