Points.value now propagates lower exceptions unchanged instead of ... #181
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Push message: Points.value now propagates lower exceptions unchanged instead of trapping and re-raise as 'Exception'.
(sorry for the verbosity, I added a short version)
Short version:
The function Points.value (in Points.py) traps all Exceptions (and derived types) to re-raise them as Exception, obfuscating the sub-class which could serve to learn the cause of the exception via “except: className” . The proposed change replaces this by a simple generic re-raise to propagate the exception higher on the stack. This has no impact on existing users, but brings filtering possibilities to take different actions based on the type of exception. For us a “device not responding” exception is handled by a wait instead of a hard stop like the other exceptions (like, for example, unknown property).
Long and boring version: (I can't believe all my search resulted in only 2 line changed)
Context: We use BAC0 to do constant, long-term, monitoring of devices. This is done via an array of threads (one per device) that poll the data points every 30 seconds. All is good.
When a devices is temporary unresponsive, (power-outage, software maintenance, network issues) an exception is thrown (details in note-5, later). To recover from this, we trap the Exception and continue.
Problem:
The function Read.read(), that raises the exception can raise 4 different types of exceptions: ApplicationNotStarted, UnknownPropertyError, UnknownObjectError, NoResponseFromController.
Unfortunately the higher layer Point.value() traps all those exceptions via the base class Exception and re-raise a type Exception, making it error-prone to detect a NoResponseFromController from the other exceptions. E.g.: no-response must be detected via text parsing instead of the intended “try: / except:” mechanic.
Proposed solution:
Change the Point.value function to not re-raise a generic Exception, but instead do a simple call to “raise” to propagate higher the received exception.
Impact analysis:
The proposed change has no impact, since anyone trapping the exception must be doing so using the class Exception which will still work. The change offer more options to the user who can now filter based on the exception class being raised.
Note-1: The proposed change creates an empty “except:” block with no purpose. The code may be cleaner by removing the “try:” block entirely and let the exception be trapped higher, but after doing so I found the presence of the “try:” to have a decent documentation value for the future. If BAC0 ever wish to re-work a related mechanics internally, this would be the right place to do so, clearly indicated by the try/except block.
Note-2: This arguable breaks a best practice “don’t use exception as flow-control”, but since the exception occurs via a call to a “get-attribute” functionality there is no other choice than to use an exception to handle the problem.
Note-3: I did not change all the “try/except/raise Exception” found in Point.py, as I was unsure of the impact for other cases. A search on "raise Exception("Problem reading” reveals all cases.
Note-4: The same change is applied to “class BooleanPoint” for the same reasons.
Note-5: Details of the thrown exception
When Point.value() is called to retrieve the property “value” for said point, it makes a call to Read.read() which creates a ReadProperty.read() request, which upon returning an iocb.ioError will attempt a diagnosis and raise a NoResponseFromController as a default case for the reported error.
References: (some of my readings for this)