Fix the python session to return correct error message when invalid session for MI drivers#2181
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #2181 +/- ##
==========================================
- Coverage 89.85% 89.65% -0.20%
==========================================
Files 73 73
Lines 19006 19048 +42
==========================================
+ Hits 17077 17078 +1
- Misses 1929 1970 +41
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
zoechanzy
force-pushed
the
users/zchan/fix-invalid-session-error-description
branch
from
3c6da41 to
ab14a93
Compare
| error_string = self.error_message(error_code) | ||
| return error_string | ||
| except errors.Error: | ||
| pass |
There was a problem hiding this comment.
It is possible that the session is valid but the returned_error_code unequal to error_code
If this happens, then something likely went wrong retrieving the error message. I'm not sure calling error_message instead of get_error_description is going to change anything in that case.
There was a problem hiding this comment.
Calling get_error_description again might cause infinite loop.
There was a problem hiding this comment.
No, I wasn't suggesting that we call get_error_description again. I'm merely trying to think about what behavior we'll see if this new section of code executes.
Oh, i see. Sorry, I meant get_error, not get_error_description.
There was a problem hiding this comment.
I also find this comment a bit confusing....
What I understand is get_error reads what is currently in the session, if it is overwritten or cleared it could still successfully run but will return mismatched error code.
So this try block is to check the error_message in the current session first before proceed to the set_session_handle in the next try block.
There was a problem hiding this comment.
How about wording the comment to be like this?
# get_error reads the session's error queue, which may have been overwritten,
# causing it to return a mismatched error code. error_message takes the error
# code directly as a parameter and looks up its description without reading the
# queue, so it will just return the description for the specific error code.
# Try it here with the current session before the handle reset in the next block.
| self.set_session_handle() | ||
| error_string = self.error_message(error_code) |
There was a problem hiding this comment.
Did you add the other error_message call to protect against resetting the session handle in instances where the session is valid but we fail to retrieve the error message?
There was a problem hiding this comment.
If the scenario is valid session but failed to retrieve error message, it will enter third try block. Resetting the session will not affect its error_code so it will flow into the error_string = self.error_message(error_code) as well. It will still fail to retrieve the error message since the error_code is unchanged. So, it will raise execption and return "Failed to retrieve error description"
If the scenario is invalid session, it will reset the session in third try block and self.error_message(error_code) will not raise execption because session is set to 0 already. So, it will return correct "The session handle is invalid" message.
There was a problem hiding this comment.
If the scenario is valid session but failed to retrieve error message, it will enter third try block.
I'm not sure that's true.
I think the 2nd try block will return from get_error_description with an empty error string.
Have you done testing to determine the behavior?
If you believe we could enter the third try block and reset the session when the session is valid and we fail to retrieve the error message, then you shouldn't be resetting the session. Failure to retrieve an error message does not justify leaking a valid session handle.
There was a problem hiding this comment.
I think the 2nd try block will return from get_error_description with an empty error string.
Have you done testing to determine the behavior?
I tested with :
- Unknown error code (-9999999999) → get_error_description returned "Explanation could not be found for the requested status code."
- VISA layer error code (-1073807339 )→ get_error_description returned "VI_ERROR_TMO: Timeout expired before operation completed."
Since the real driver does not raise errors.Error from error_message() on a valid session either, I used a mock to cover the except errors.Error in second try block and third try block.
with patch.object( type(session._interpreter), 'error_message', side_effect=errors.Error(real_error_code) ): description = session._interpreter.get_error_description(real_error_code)
It returns "Failed to retrieve error description."
Failure to retrieve an error message does not justify leaking a valid session handle.
I will update code to save the session handle before resetting it and restore it afterward to avoid leaking a valid session handle. We cannot selectively restore only valid sessions because we can only determine if a session handle is valid by calling a C driver function and observe whether it succeeds or fails.
|
FYI @zoechanzy , our Linux VM is currently in bad shape, so Linux System tests are going to fail. We hope to have it resolved in the next week or two. |
|
Requested to update description to be more descriptive of the error. Thanks! |
|
Propose to rename the title to be In the PR description Can you explain with screenshot, something like: The expected error message when invalidated NI-SWITCH session, the error message is different between Labview and Python
|
zoechanzy
changed the title
| (IVI spec requires GetError to fail). | ||
| Use error_message instead. It doesn't require a session. | ||
| ''' | ||
| save_vi = self.get_session_handle() |
There was a problem hiding this comment.
Hmm you are defining and calling self.get_session_handle() here but reference in the finally. My concern if get_session_handle() raises anything other than errors.Error, it may skip the rest of the try block, and then when run finally block the save_vi is unbound.
I think maybe we should move this save_vi = self.get_session_handle() before the try block?
save_vi = self.get_session_handle() # moved outside try
try:
self.set_session_handle()
error_string = self.error_message(error_code)
return error_string
except errors.Error:
pass
finally:
self.set_session_handle(save_vi) # always safe or not run if save_vi failed
There was a problem hiding this comment.
Hmm I notice the _library_interpreter.py files on all drivers are also written like this, I not sure how the generated folder files being written, I suppose we only need to change this file and run some local build/script it will update for the _library_interpreter.py files for all drivers?
There was a problem hiding this comment.
I suppose we only need to change this file and run some local build/script it will update for the
_library_interpreter.pyfiles for all drivers?
@jfan-1995 I updated the py.mako file only and ran tox command to generate all the files and run unit test.
6 participants
What does this Pull Request accomplish?
get_error(reads session error queue)If the session is valid and the error queue has not been overwritten, returned_error_code matches error_code and we return the description immediately.
Reached if Block 1 fell through — either
get_errorraised, or it returned a mismatched code (queue was overwritten). Unlikeget_error,error_messageis a direct lookup byerror_codeand does not read the error queue. If the session is still valid, this returns the correct description without touching the session handle.( second try statement is exactly same with the second try statement in original file. I only updated the docstring section).
Reached only when Block 2 also failed — meaning the error_message in TRY block 2 fail to retrieve error due to internal error or it is an invalid session.
error_messagerejects calls with an invalid session handle, but succeeds withvi=0.List issues fixed by this Pull Request below, if any.
What testing has been done?
Test of invalid session (this bug after fixing):
Unit tests and flake8 passed.