-
Notifications
You must be signed in to change notification settings - Fork 50
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
nidaqmx-gRPC Local Thread Memory (GetExtendedErrorInfo) #755
Conversation
NOTE: A pull was done to update my local main branch and it looks as if GetExtendedErrorInfo is not incorporated in the examples for nidaqmx. It is seen however in some generated services and clients. Due to this I chose to insert a "codegen":"private" |
@reckenro any idea why the python validation is failing? It's not clear to me what's even causing it. |
Yeah, think one of our dependencies latest versions updated yesterday which is causing issues. I have a fix in PR 752 where I was also hitting this: https://github.com/ni/grpc-device/pull/752/files#diff-de4bc5702b4e1bd6b0ed30ebefbd2d657f568a8dd2a845f39a352fefe3de9f1d |
…_extended_error_Bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. One thing that I'm less familiar with is updating it in scrapigen: https://github.com/ni/grpc-device-scrapigen It's currently what produces this grpc-device metadata for nidaqmx.
We can get @DavidCurtiss or @astarche 's thoughts on if we should update scrapigen for this (I'm guessing yes) and possibly more info on the how.
Yes, we should. If it would work to just remove the function, then the best path forward is to copy the function from DAQmx.h into DAQmx_exlcude.h. But if it's important to keep it present but mark it private, we'd write special code to do that in src/scrapigen/daqapi/filterfunctions.py. |
@harsha-bhushan If you have some time I was wondering if I can have your thoughts on this PR. Thanks! |
Sorry, I got this question after I approved the PR. Wouldn't there be a similar behavior in C API as well when the function is used the way shown in the bug? What am I missing? |
@harsha-bhushan what you're missing is that for gRPC-device the server makes calls on unpredictable threads. In the C API it is not a problem because one's code always executes sequentially on the same thread. So you get back an error, and you check for the details on the same thread. But that's not possible on gRPC-device because even if in your client-side code you make the calls on the same thread, that's not the same where the code actually calls the driver. |
@bkeryan Brad I was wondering if you could take some time to look at this bug and be able to close it out on the GitHub side. Right now it is still pending validation. |
What does this Pull Request accomplish?
This Pull Request acts on the Issue 684 (#684) removing the use of GetExtendedErrorInfo to get errors as they use thread local storage and instead utilizing grpc error codes.
Breaking changes
Why should this Pull Request be merged?
This should be merged to make GetExtendedErrorInfo unusable by the users as it will generate unwanted error responses.
What testing has been done?
Local builds have been done to ensure building is functional and testing of getting a error locally was done as well.
AB#2085088