Make treatment of exceptions more pythonic

[chintal]

Versions

• Pymodbus: 1.4.0+ atleast, probably historic versions as well

Pymodbus Specific

• Server: Probably unaffected, though I can't quite say for sure
• Client: observed in sync, though probably applies to async as well

Pymodbus (mis-)handles exceptions, presenting a non-intuitive interface and in some cases potentially dangerous behavior in terms of exception handling. This Issue lists out some of the problems I have noticed over the last few weeks of my use of pymodbus, with the hope that it may result in a discussion leading to viable approaches to better handle exceptions.

Current Behavior

Pymodbus doesn't often raise exceptions except in cases of catastrophic failure. Instead, it returns an exception response in place of a regular response. This is functionally sufficient, but in practice this means that every command sent from a client to a server needs to be checked for an exception response. This makes pymodbus code look and feel very unpythonic, and more verbose than it ought to be.

Types of exceptions returned and their detection

As far as I can tell, a pymodbus client call might result in one of two kinds of exception responses :

1. An ExceptionResponse, returned from and indirectly by the modbus server. This is, in all cases, a real exception. The server was asked to do something which it refused to do. Such a response should always be treated as an exception.
2. A ModbusIOException, raised by the Client itself, implying that no ModbusServer responded to the command issued. This is not always a real exception - some (one, ForceListenOnly) Modbus commands return no response whatsoever. All modbus server return no response for any command issued in broadcast. However, the client issuing the command knows or can know at the time of its issual whether or not it should expect a response. If this information is made available to the client or its command executor, it can determine if a returned ModbusIOException is a real exception or something which can be silently discarded (as no response was expected anyway). On a side note, ModbusIOExceptions should in principle also result from a failure of the client to send the message, such as if, say, the tty interface disappears mid-message. I'm not sure what the current handling of exceptions does in such a case.

Exception Raising

A modbus client should, in my opinion, raise an exception that is determined to be a real exception and not just return it with the expectation that user code will detect and respond to the problem. There are two dimensions to solving such a problem, viz.,

a. How do you know an exception should be raised?
b. How do you actually raise an exception?

(a) is addressed previously. Additional work might be needed to embed information about whether a response should be expected in the case of (2), but exceptions of type (1) can be immediately raised.

(b) is a relatively more complex question. In the case of the sync clients, raising an exception is fairly trivial. In fact, for exceptions of type 1. in sync clients, all that is needed to change behavior is :

class ModbusServerException(Exception):
    def __init_(self, response):
        self.response = response

class RaisingModbusClient(ModbusSerialClient):
    def execute(self, request=None):
        result = super(RaisingModbusClient, self).execute(request)
        if isinstance(result, ExceptionResponse):
            raise ModbusServerException(result)
        return result

For the type 2. exceptions, more code would be needed here, such as, say :

...

class RaisingModbusClient(ModbusSerialClient):
    def execute(self, request=None):
        ...
        if isinstance(result, ModbusIOException) and request.expect_response:
            raise result
        ...

In the async clients, this problem can be relatively more intractable. It has been some time since I've used twisted, and I have, in the past, made an effort to avoid the complexity of errbacks. However, the correct way for pymodbus to handle exceptions in it's async clients should be to accept errbacks and return exceptions though them, once detected as discussed above.

Maintaining Backward Compatibility

Such a change would cause some fairly severe backward compatibility issues, given all pymodbus users don't expect it raise exceptions and instead to return them. As such, I would suggest instead implementing such behavior but leave it disabled by default. Allow each user to switch to the new format at their choice. This can be done by providing additional keyword arguments to the Client class' init() methods, tentatively raise_exceptions=False, which defaults to the old (current) behavior. A user may wish to use raise_exceptions=True and then not have to check every response for an exception.

In the interim, or even beyond, given the obvious complexity of the type 2. exception detection, the provided arguments may instead be :

• raise_exceptions=False
• raise_server_exceptions=False
• raise_io_exceptions=False

This will preserve enough flexibility for a user to choose not to rely on pymodbus for determining the context of ModbusIOExceptions, but allow it to raise the certain exceptions of ExceptionResponse.

[dhoomakethu]

@chintal thanks for the detailed explanation . I know it is quiet confusing to start with to understand why pymodbus does not raise exceptions on failure but returns ModbusIOException and There is no definite this is the way it needs to be handled as well. At present (v1.5.0), ModbusIOException is raised when there is no response received or when malformed packets are received resulting in the processing failure. I agree in some cases (ForceListenOnly) having ModbusIOException is not the right way to go ahead and we can add this as an enhancements. Also, each valid modbus response(success/exception) object returned has isError() method which should give an idea whether the received response is a success or an exception response. I can add this to ModbusIOException object as well and return always True so that it could be verified before proceeding further.
Having said that I will see if I can incorporate your suggestions in the current code base to make it more explicit and give more control to the user.

    res = client.read_holding_registers(...)
    if not res.isError():
        # proceed
    else:
        # handle error or raise
    ```

[chintal]

@dhoomakethu It would be nice if this would be how such code could look, if necessary by a user explicitly throwing some switches during Client instantiation :

try:
    res = client.read_holding_registers(...)
    # Do stuff with res
except (ModbusServerException, ModbusIOException):
    # exceptions were raised only when the exception was a 
    # real problem and not an expected silence.
    # handle error or raise

The try block can grow to as large or as small as is needed for your exception handling process. For a number of cases, the block could be as large as a complex function, with many related modbus calls, or in simpler cases encapsulating a whole script, simply telling the user to check connections and exit.

[d4ddi0]

Older versions of pymodbus returned None when they didn't get a valid response, which was also (in my view) more pythonic that the current behavior. Upgrading to 1.4.0 broke my stuff...(so I'm all for backwards compatibility going forward)

I also feel like raising exceptions is the proper way to go, but for backwards compatibility, I would suggest keeping the the current functions exactly as they are, but deprecating them in the next release.
returning Error objects is very weird (i.e. nonintuitive) behavior in python.

[memetb]

For what it's worth, I've previously expressed a preference for raising exceptions as well.

While the additions of isError are useful and fulfill the role, it would be nice to have a future roadmap where exceptions are done in the pythonic way. except and finally are very useful constructs which simply can't be taken advantage of in the current code.

Perhaps a kwarg flag during client creation that enables exceptions? (although I recognize this will create spaghetti code internally)

[dhoomakethu]

I have added this issue to 2.0.0 milestone . I will let you guys know for a code review when an initial draft is ready. I am ok with what @chintal has proposed (with explicit arguments to control the exceptions either being returned or raised).

[chintal]

@memetb There should not be any significant spaghetti code created internally. Whether pymodbus chooses to manage its own internal control flow with or without exceptions does not necessarily come through to a user of the library. The internal changes, if required, can be done gradually with no breaking API changes. The proposed solution is to make the 'standard' interfaces to the library behave more 'pythonically', ie, assume that exceptions and errors can be passed around perfectly well within the library just as long as an appropriate exception is raised by a client's execute method.

[mihai-dobre]

2 years later we still need the module to behave in a more pythonic way when it comes to exceptions. Another problem I've identified is the fact that the exceptions are not actual exceptions( they not inherit Exception ). So there is literally a nightmare to do exception handling. I know, I'm complaining, but I'm also willing to help if possible.