inquiry into mutex locking changes in `Device` and `Queue`

[diablodale]

I'm unsure of some mutex locking changes in commit 55066c5

dai::DeviceBase::closed changed from atomic<bool> to pair of std::mutex + bool

The two places I find these vars are used DeviceBase::isClosed() and DeviceBase::close() correctly implement the mutex+bool pair.
My concern is that there is no known case for this code change and no functional bug.
This change adds code complexity, more cpu, and additional memory use when not needed.
And finally, it makes a substantial behavior change. This change creates a blocking behavior where the original code did not.
Now multiple threads which call these functions will block rather than quickly returning.
Since this is a blocking behavior change, this is a risky change for no known need.

When I refactored this code months ago, I had my head around the locking needs of the multiple threads running. At that time I felt the atomic<bool> was the better choice for simplicity and its non-blocking behavior. It didn't matter which thread closed a device; it only mattered that one thread ran the close code. I acknowledge that I can be wrong and write bugs. But that isn't the case here as there is no known bug and no imagined case.

int DeviceBase::getXLinkChunkSize() {
    checkClosed();

    return pimpl->rpcClient->call("getXLinkChunkSize").as<int>();
}

checkClosed() there is useless.
It calls isClosed() which locks the mutex and returns the bool value of closed.
If it is true then checkClosed() throws which then prevents the call to pimpl->rpcClient->call().
But the checking of the value and the throw are disconnected making the test useless.

Time	Thread 1	Thread 2
1	DeviceBase::getXLinkChunkSize()
2	checkClosed() returns false
3	…os pauses thread…	DeviceBase::close()
4	…os pauses thread…	std::unique_lockstd::mutex lock(closedMtx);
5	…os pauses thread…	if(!closed) {
6	…os pauses thread…	closeImpl();
7	…os pauses thread…	closed = true;
8	pimpl->rpcClient->call("getXLinkChunkSize").as();
9	call(…)
10	💥 or something fails/bad/errors/etc

Changing the behavior of the closed/closedMtx to be blocking doesn't help any of the cases that I can readily find. Instead, the above commit causes mutex locking and blocking of threads in the multithreaded world of depthai+xlink sending/receiving packets and making RPC calls. I think this change should be reverted.

dai::XLinkConnection::closed changed from atomic<bool> to pair of std::mutex + bool

Same everything as above. No known need, more complex, and behavior change to blocking.
XLinkConnection::isClosed() is now blocking. Therefore XLinkConnection::checkClosed() is now blocking.
Suprisingly, I didn't find any code calling XLinkConnection::checkClosed() so I don't have a code example.
I also think this change should be reverted.

ai::LockingQueue::destructed changed from std::atomic<bool> to to pair of std::mutex + bool plus redundant if() tests

Same everything as above. No known need, more complex, and (less concerning) behavior change to blocking.
I intentionally removed the if(destructed) and you agreed in my commit fc4b314.
Comments start at #366 (comment)
I don't now have the entirety of that codebase in my head. I would need time to recreate (ugh) my knowledge.
But I trust the decision we both made in Feb...unless you have a specific bug or imagined scenario. 🤔

[Luxonis-Brandon]

Thanks for all the details here!

[themarpe]

Hi @diablodale

I'll only address the last one currently: #509 (comment)
Let me know if that describes the issue at hand.

Will revisit first two cases later today

[themarpe]

I agree on the first point.
Check closed should take the mutex as well & then throw if its closed to work as expected.

Yes, both 1st and 2nd cases could be reverted. The imagined scenario doesn't actually require this, could be done with the previous impl (which is a known time after close actually closes).

[diablodale]

Hi. Revisiting this as I've returned from my travels. I don't see there was activity in the codebase related to this. Can I make a PR to revert some of these changes?

[themarpe]

Hi @diablodale

You may - first and second cases don't have any specific rationale behind it apart from only returning when it was closed after its actually closed

[diablodale]

@themarpe , hi. I've got time this week to work on this. I see checkClosed() has spread and in the majority of cases it is useless. None of those cases are thread-safe and the check/throw has a random outcome. I give one of many possible threading scenarios in the OP above.

I recommend the code be fixed as any safety intended by checkClosed() doesn't work. Often, the authors intended to implement..."I need to run some code, but my code can only run if the device is closed, so check that the device is closed and throw if it is open". To implement that...a mutex lock is needed. A lock needs to be held during the entire block of code that has the restriction "...my code can't run if the device is open...". All call locations may not have this same intention. Each call location needs to be analyzed to see if there are other intentions. I see ~50 locations. 🫤

I need to look more, but I have a gut worry around the restriction: "the device is closed" or is it "the device is not open". This might be coded differently in the underlying booleans and mutex. I have to review the code.

If that worry is false, then I might be able to reduce refactoring by using a movable lock like unique_lock and change checkClosed() to return an r-value of that lock and use [[nodiscard]]. The latter is c++17 only so this is error prone...even more since core devs have gotten used to errant usage. Since you want to support c++14, it is probably better to delete the bad checkClosed() function and create a new one like getClosedLock() that returns the r-value and heavily hints there is a return.

Comments before I start coding?

[diablodale]

Also problematic is that isClosed() is a public function on several classes; and it is exposed to both C++ and Python.
isClosed() is not thread-safe and I recommend removing it completely.
Until you approve the removal, I'll doc/mark the API strongly to discourage usage and provide warnings.

[themarpe]

Hi @diablodale

I need to look more, but I have a gut worry around the restriction: "the device is closed" or is it "the device is not open". This might be coded differently in the underlying booleans and mutex. I have to review the code.

The intention is to error out if device was already closed/disconnected.

I agree on it not being thread safe, at the time was mostly a quick add, to see if device went down already.

I think would be best that underlying RPC calls themselfs would check for that & properly error out, that way there would be no need to hold the lock until rpc is finished, etc...

[themarpe]

WRT isClosed - main intention is to check wheter or not the device connection is still live.
The removal of it is not planned, and would be better to address the shortcomings of it at the current state

[diablodale]

WRT isClosed - main intention is to check wheter or not the device connection is still live. The removal of it is not planned, and would be better to address the shortcomings of it at the current state

ok. Callers that spin on isClosed() with a wait(1s) as seen in some examples are not a concern. The concern is the other callers that do active work with the expectation that the device in up and running. Pseudocode...

while (not device.isClosed()) {
  device.makeStateChanges()
  device.push(data)
  device.pull(data)
}

Imagine that above in python. There's nothing that prevents this scenario...

python interpreter	some other thread
call isClosed()
receive FALSE from isClosed()
	set running=false, deallocate all device/queue resources, etc.
logical NOT(FALSE) --> TRUE
while(TRUE) loop
device.makeStateChanges()
💣 throws, crashes etc.

Given isClosed() is unreliable/random, I almost think it is better to write the loop as while(true) and let the throw/crash cause the loop exit.

[diablodale]

I also want to highlight a scenario that I see repeatedly, provide a suggestion, and ask your feedback so that I know what to do throughout the codebase.

depthai-core/include/depthai/device/DataQueue.hpp

Lines 144 to 155 in e5dae8f

	/**
	* Try to retrieve message T from queue. If message isn't of type T it returns nullptr
	*
	* @returns Message of type T or nullptr if no message available
	*/
	template <class T>
	std::shared_ptr<T> tryGet() {
	if(!running) throw std::runtime_error(exceptionMessage.c_str());
	std::shared_ptr<ADatatype> val = nullptr;
	if(!queue.tryPop(val)) return nullptr;
	return std::dynamic_pointer_cast<T>(val);
	}

Two issues in this code:

1. if(!running) throw... is not thread-safe. The running state can change any nanosecond after getting the low-level boolean like in the negation, in the if evaluation, in between it and the throw, etc.
2. The whole function may not be thread-safe. What is the intention with regards to DataOutputQueue state while this function is called? Is it...
   a. "Allow close/deallocating resources while I try to retrieve message T from those resources (e.g. queue)"
   b. "Prevent close/deallocating resources while I try to retrieve message T from those resources (e.g. queue)"

If the intention is 2b...then a mutex lock is needed so that close() can't proceed at the same time tryGet() is running. I don't think this specific scenario will cause a crash since the queue has its own internal mutex locks. Instead, this is more a semantic/logical inquiry so that I understand the intention

[diablodale]

I think would be best that underlying RPC calls themselfs would check for that & properly error out, that way there would be no need to hold the lock until rpc is finished, etc...

Reviewing the develop codebase, I think the rpc functionality protects itself and throws when bad things happen. Here's what I've seen in code.

• DeviceBase::init2() creates the pimpl->rpcStream and pimpl->rpcClient
• pimpl->rpcStream retains a std::shared_ptr<dai::XLinkConnection> which it copied during construct. The XLinkConnection won't ~destroy itself due to this shared ownership. Naturally, a connection can fail due to unexpected problems.
• pimpl->rpcClient lambda retains a copy of the pimpl->rpcStream. That specific xlinkstream will not ~destroy itself due to this shared ownership. And remember that specific xlinkstream contains the shared XLinkConnection.
• pimpl->rpcClient lambda calls rpcStream->write() and ->read(). Both of them will throw if they encounter any low-level XLink errors. So if at any time the XLinkConnection and/or XLinkStream die, then these two function calls should throw. The throw will cascade up the call hierarchy. First out of the lambda and to the code that called rpcClient->call().
• Many (not all) of the places checkClosed() is used are within functions that use RPC. For example, DeviceBase::setTimesync() calls pimpl->rpcClient->call("setTimesync", .... The throw I describe directly above will cascade up from within the rpcClient->call() and then further up. This throw will be throw XLinkWriteError(status, streamName) instead of the checkClosed throw std::invalid_argument("Device already closed or disconnected");. Personally, I'm ok with the low level throw and I also don't think invalid_argument is appropriate since the timesync argument was valid...it was something else that failed. But if you want, it is possible to wrap all the pimpl->rpcClient->call() call locations with a try/catch and throw whatever you want.

There is likely an issue with blindly calling pimpl->rpcClient->call(). It is the basic issue of calling a function through a nullptr. If some thread or code somewhere calls one of these DeviceBase functions like DeviceBase::setTimesync() while something else has closed() the Device and the C++ runtime is destructing the pimpl, then depending on when setTimesync() is called then the rpcClient and/or the pimpl itself can be invalid. A hack for that is to check if pimpl != nullptr before ->rpcClient and then again before ->call(). But that's a race condition as another thread can again be destructing pimpl or DeviceBase. To safely call, I think a mutex is needed to prevent the pimpl and rpcClient pointers from becoming invalid.