pysunspec is not thread safe

[bbinet]

I've built a simple web application based on pysunspec and bottle to be able to query sunspec data through http requests. It works very well when I use a single-threaded server like Python wsgiref, but I noticed that when I use a multi-threaded server like cherrypy, the following errors start to occur:

Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/bottle.py", line 862, in _handle
    return route.call(**args)
  File "/usr/lib/python2.7/dist-packages/bottle.py", line 1729, in wrapper
    rv = callback(*a, **ka)
  File "/usr/lib/python2.7/dist-packages/sunspecweb/server.py", line 13, in wrapper
    return func(*args,**kwargs)
  File "/usr/lib/python2.7/dist-packages/sunspecweb/server.py", line 125, in get_device_model
    m.read()
  File "/usr/lib/python2.7/dist-packages/sunspec/core/client.py", line 336, in read
    self.model.read_points()
  File "/usr/lib/python2.7/dist-packages/sunspec/core/client.py", line 197, in read_points
    data = self.device.read(self.addr, self.len)
  File "/usr/lib/python2.7/dist-packages/sunspec/core/client.py", line 82, in read
    return self.modbus_device.read(addr, count)
  File "/usr/lib/python2.7/dist-packages/sunspec/core/modbus/client.py", line 453, in read
    data = self._read(addr + read_offset, read_count, op=op)
  File "/usr/lib/python2.7/dist-packages/sunspec/core/modbus/client.py", line 407, in _read
    c = self.socket.recv(len_remaining)
AttributeError: 'NoneType' object has no attribute 'recv'

Is pysunspec supposed to be thread safe?

[bobfox]

Currently there is no lock or queuing for device access. Because of this, the device access portion of pysunspec is not thread safe.

[bbinet]

Ok. Do you plan to implement thread safety?

[bobfox]

It is not currently planned but if it is needed we can look at it. We also welcome any contributions to the library to make it better.

[bbinet]

I think it would be a useful improvement. I would be happy to contribute, I'm not used to thread safety issues but I will try.

[bbinet]

I tried to play with threading.Lock to protect portions of the code which read or write on the socket, but it does not seem to help... Could you provide any guidance on how to protect against those thread safety issues?

[altendky]

@bbinet It's been awhile, but why and how are you using threads? While they do have their place, threads are often discouraged in Python in favor of other async approaches. Note that using threads does not gain you more CPU time in Python despite that being one of the common reasons for using threads in other languages.

Perhaps instead of thread safety it would be more valuable to add support for the Twisted and/or asyncio libraries. I personally use Twisted for my CAN communication.

[bbinet]

@altendky I've tried the threading approach because I didn't want to introduce a new dependency. We could try asyncio, but it would mean we would drop Python 2 support...

[altendky]

@bbinet Twisted supports both Python 2 and 3. But, for threading I'd probably wrap up pysunspec in an object that would sit in it's own thread with a queue to feed it with requests. Then you'll probably need queue(s) back to your other thread(s) to get the results. Remember that it's not just that you need to avoid multiple access to the hardware but that you need to maintain the order of handling requests and responses.

[altendky]

@bbinet and @bobfox, in case you were interested I have added preliminary support for Twisted along with a sample script.

https://github.com/altendky/pysunspec/blob/a04f5c8f77432ed3df45eeee8990ff53698818c2/sunspec/scripts/async.py

The script will install in the Python scripts directory when you install the package and can be called such as:

sunspecasync twisted --name COM4 --write epc_control CmdRealPwr 42

It also provides a classic command along with twisted so you can compare the implementations. epc_control is a model in my case, CmdRealPwr is a point inside it, and 42 is a sample value to write. It does attempt to write back the original value after writing the provided value. If you don't pass --write then it only demos reading. If you don't know your com port name:

sunspecasync list ports

There is a bug in Twisted relating to pySerial >= 3.0 on Windows and another for Python 3 (which sunspec proper doesn't support anyways, but my branch does). Either use my Twisted branch or just avoid pySerial>=3/Python3 when on Windows.

As I said, this is preliminary but feel free to try it out and let me know what issues you have. I haven't used it beyond the test script and I know I need to do lots of refactoring to avoid the amount of copy/paste I have now.

[altendky]

For completeness, the Twisted issues are below. I haven't contributed to Twisted before so I don't know how long it might take to get my patches accepted. Sadly, one of them was previously open for 18 months without getting a sufficient quality patch submitted.

http://twistedmatrix.com/trac/ticket/9186 http://twistedmatrix.com/trac/ticket/8159

[bbinet]

Thanks @altendky, that's great! I won't have time to test your branch in the near future, but I'll report back the results here when I'll find the time.

[altendky]

Also: https://twistedmatrix.com/trac/ticket/9252

But, all changes have been accepted and I just ran the script with Twisted trunk. There was talk of a release at the beginning of the month but that hasn't happened yet. Maybe soon.

[altendky]

I haven't touched this for a long time but it seems that the sensible way to provide both sync and async interfaces is to only build the async and then use something like crochet to create a sync interface without repetition.