-
Notifications
You must be signed in to change notification settings - Fork 52
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
codegen: Report DAQmx event registration failure by closing stream #927
codegen: Report DAQmx event registration failure by closing stream #927
Conversation
This is required to report rich errors in async callback methods. Signed-off-by: Brad Keryan <[email protected]>
Signed-off-by: Brad Keryan <[email protected]>
The NI gRPC Device Server uses server streaming to return a response for each event generated by DAQmx. However, when a DAQmx event registration function (like DAQmxRegisterDoneEvent) returns an error, the server reports the error by sending a streaming response where `response.status` is the error code. Implications: - It is difficult for clients to synchronously detect whether event registration failed. - In the success case, no responses are generated until DAQmx notifies the server of an event, so waiting indefinitely for a response only works in the error case. - Waiting indefinitely for initial metadata does not guarantee that the error will be reported at the same time because the server sends initial metadata before reporting the error. - Rich error descriptions are not reported for event registration errors, only an error code. - For done events, a fatal `response.status` is ambiguous: it can mean that event registration failed or that the acquisition/generation encountered an error. - The server does not close the stream when a event registration error occurs, but it will never send any responses other than the one containing the error code. Reporting event registration errors by closing the stream solves these problems: - After the client waits for initial metadata, they can check whether the event stream is done/closed without any additional waiting. - When an event registration error occurs, the event stream is closed with a gRPC status containing the rich error message and `ni-error` trailing metadata indicating the error code. - The done event stream is only closed when an event registration error occurs, when either side cancels, or when gRPC closes it for another reason. It is not closed when the DAQmx acquisition/generation encounters an error. - The server closes the stream, so clients know to stop reading events. This is a breaking change. However, it only affects error cases, and clients should already handle gRPC errors when reading the event stream. Signed-off-by: Brad Keryan <[email protected]>
Signed-off-by: Brad Keryan <[email protected]>
…orting Update the test that generates `DONE_EVENT_ALREADY_REGISTERED_ERROR` to expect a C++ exception. Signed-off-by: Brad Keryan <[email protected]>
Signed-off-by: Brad Keryan <[email protected]>
Signed-off-by: Brad Keryan <[email protected]>
When the server returns a gRPC status before sending initial metadata, it sends what the gRPC over HTTP2 specification calls a "Trailers-Only" response. If something caused this not to work (implementation change, HTTP2 proxy, gRPC-Web proyx?), the error would still be reported to the client when the client's worker thread reads the event stream. However, the client's worker thread needs to have a way to report event stream errors anyway, because there many other reasons the stream could be closed besides event registration errors. |
Add error checks and `StartSendInitialMetadata()`. Pass the request as a parameter like `nidaqmx_service.cpp` does. Signed-off-by: Brad Keryan <[email protected]>
Signed-off-by: Brad Keryan <[email protected]>
Signed-off-by: Brad Keryan <[email protected]>
Signed-off-by: Brad Keryan <[email protected]>
Ensure that error reporting and OnCancel() don't call Finish() twice. Also use move semantics like Finish() does. Signed-off-by: Brad Keryan <[email protected]>
Signed-off-by: Brad Keryan <[email protected]>
Signed-off-by: Brad Keryan <[email protected]>
Signed-off-by: Brad Keryan <[email protected]>
…-event-registration-errors
Signed-off-by: Brad Keryan <[email protected]>
Signed-off-by: Brad Keryan <[email protected]>
What does this Pull Request accomplish?
The NI gRPC Device Server uses server streaming to return a response for each
event generated by DAQmx. However, when a DAQmx event registration function
(like DAQmxRegisterDoneEvent) returns an error, the server reports the error by
sending a streaming response where
response.status
is the error code.Implications:
failed.
server of an event, so waiting indefinitely for a response only works in the
error case.
will be reported at the same time because the server sends initial metadata
before reporting the error.
an error code.
response.status
is ambiguous: it can mean thatevent registration failed or that the acquisition/generation encountered an
error.
but it will never send any responses other than the one containing the error
code.
Reporting event registration errors by closing the stream solves these problems:
stream is done/closed without any additional waiting.
gRPC status containing the rich error message and
ni-error
trailing metadataindicating the error code.
when either side cancels, or when gRPC closes it for another reason. It is not
closed when the DAQmx acquisition/generation encounters an error.
This is a breaking change. However, it only affects error cases, and clients
should already handle gRPC errors when reading the event stream.
Why should this Pull Request be merged?
Fixes #924
Prerequisite for fixing AB#2395958: GrpcStubInterpreter does not report event registration errors to caller
What testing has been done?
Manually tested with updated Python examples, which will be in a separate PR.
Ran updated C++ tests with
SystemTestsRunner.exe --gtest_filter=*DAQmx*