Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Points.value now propagates lower exceptions unchanged instead of ... #181

Merged
merged 1 commit into from
Jan 11, 2020

Conversation

IntoCpp
Copy link

@IntoCpp IntoCpp commented Jan 9, 2020

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)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 47.563% when pulling a088202 on IntoCpp:master into 17335bc on ChristianTremblay:master.

@coveralls
Copy link

coveralls commented Jan 9, 2020

Coverage Status

Coverage increased (+0.5%) to 47.563% when pulling a088202 on IntoCpp:master into 768fda8 on ChristianTremblay:develop.

@ChristianTremblay
Copy link
Owner

Thanks. I will read this carefully.

Regarding the control flow vs Try/Except... Python encourage this and exceptions are often used for flow control (StopIteration for example in For loops or in asynchronous generators)...

I see no problem there.

I will change the target to merge in develop

@ChristianTremblay ChristianTremblay changed the base branch from master to develop January 10, 2020 03:02
@ChristianTremblay ChristianTremblay merged commit 6e21617 into ChristianTremblay:develop Jan 11, 2020
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