Skip to content

Conversation

susnicek
Copy link

@susnicek susnicek commented Mar 6, 2025

This commit adds a shared threading lock in constructor of the class with a default value in case it is not necessary. This enables users to use other instruments that are handled in threads. The threading lock is applied on _communicate function, so the placement of the lock can be further optimized. However in the first approximation this code works.
The correct utilization is as follows: the user creates an instance of the Threading.lock. Than the this instance is passed to the all instances of his modbus Instruments. The Instrument objects communication with serial port can be than processed in threads and the shared lock prevents concurrent access to the port.

@j123b567
Copy link
Collaborator

j123b567 commented Mar 6, 2025

It would generally be a good idea to do a resource lock, but I have several problems with this particular implementation.

  • By default, locking is useless and just adds unnecessary overhead. You can replace the default value with contextlib.nullcontext with no significant difference.
  • It gives false hope that the library is doing proper locking of the shared resource, but it is not. The lock should protect the serial port, not the Instrument, so by default the lock should be shared per serial port, as the existing serial port is shared per Instrument. Similar mechanism is for _latest_read_times.

You can always implement simple derived class for your own project without a need to modify a code of the library.

shared_lock = threading.Lock()

class InstrumentWithLock(Instrument):
    def _communicate(*args, **kwargs):
        with shared_lock:
            return super()._communicate(*args, **kwargs) 

@shipmints
Copy link

It would generally be a good idea to do a resource lock, but I have several problems with this particular implementation.

  • By default, locking is useless and just adds unnecessary overhead. You can replace the default value with contextlib.nullcontext with no significant difference.
  • It gives false hope that the library is doing proper locking of the shared resource, but it is not. The lock should protect the serial port, not the Instrument, so by default the lock should be shared per serial port, as the existing serial port is shared per Instrument. Similar mechanism is for _latest_read_times.

100% better not to burden the code with this when users can trivially wrap their own.

@susnicek
Copy link
Author

susnicek commented Mar 9, 2025

Yes, that's right. I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants