Skip to content

Replace custom mutex implementation with std::mutex

m.marn requested to merge remove-mutex into master

#86 (closed) Current changes only replace the old custom Mutex implementation with std::mutex. There is one instance where a recursive mutex is required and I mentioned it in the code. I noticed cases where the mutex is not locked and should be. However the current changes don't yet address those issues, and functionally the code should be the same as before. The only change is that on doClose the mutex is now actually locked, while before even if withLock=true was provided the mutex wasn't actually locked.

Problems:

  • Connection::enable - isEnabled_ not locked
  • Connection::disable - isEnabled_ not locked
  • Connection::doOpen - returned isConnected_ not locked - thePLC->updateLocalData(); while not a race-condition this requires a recursive mutex since bufferMux is locked recursively.
  • Connection::reOpen - doClose and doOpen should probably be done under one lock - isActive_ is static and synchronization is not handled
  • Connection::isEnabled - not locked
  • Connection::isConnected - not locked
  • SNAP7Connection::checkError - Possibly a deadlock can happen when this function is called. The order of locking the mutex is not always the same. It can be called with the read/write mutex already locked and then it locks the connection mutex, while Connection::disable locks in a different order.

Thread.h could be removed entirely and it's usage replaced with std::async. PLC::recvAsync and PLC::sendAsync would save a std::future and PLC::waitAsync would wait on that future. With this the Condition class can also be removed since it's only used in Thread.

In general there is a lot of friend classes in the whole codebase which makes it hard to reason and understand which classes are used from where.

Merge request reports