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

codegen: Report DAQmx event registration failure by closing stream #927

Merged
merged 18 commits into from
May 25, 2023

Conversation

bkeryan
Copy link
Contributor

@bkeryan bkeryan commented May 18, 2023

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:

  • 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.

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*

bkeryan added 7 commits May 17, 2023 20:28
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]>
@bkeryan
Copy link
Contributor Author

bkeryan commented May 18, 2023

When the server returns a gRPC status before sending initial metadata, it sends what the gRPC over HTTP2 specification calls a "Trailers-Only" response. WaitForInitialMetadata waits for HTTP2 headers and it gets HTTP2 trailers instead.

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]>
@astarche astarche added the binary-breaking Change to proto file that requires client updates label May 18, 2023
bkeryan added 2 commits May 18, 2023 16:22
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]>
@bkeryan bkeryan requested review from astarche and reckenro May 18, 2023 21:31
Base automatically changed from users/bkeryan/server-context-base to main May 19, 2023 00:59
@bkeryan bkeryan merged commit 80a1333 into main May 25, 2023
@bkeryan bkeryan deleted the users/bkeryan/nidaqmx-event-registration-errors branch May 25, 2023 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binary-breaking Change to proto file that requires client updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NI gRPC Device Server reports DAQmx event registration errors as events, not as errors
3 participants