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

Oneway service request from the command line #477

Merged
merged 16 commits into from
Mar 13, 2024
Merged

Conversation

caguero
Copy link
Collaborator

@caguero caguero commented Feb 2, 2024

🦟 Bug fix

Partially fixes #97

Summary

This patch makes it possible to request a one-way service request from the command line. Before, it was possible passing --reptype gz.msgs.Empty but always getting the message Service call timed out as an output (because the service callback wasn't answering anything, which was expected).

How to test it?

You can compile the examples, and then:

  1. Launch the one-way responser:
./responser_oneway
  1. Request the service using the two-way request and verify that you don't get the timeout:
gz service -s /oneway --reqtype gz.msgs.StringMsg --reptype gz.msgs.Empty --req 'data: "Hello"' --timeout 1000
  1. Use the new one-way request and verify that you don't get the timeout message:
gz service -s /oneway --reqtype gz.msgs.StringMsg --req 'data: "Hello"'

Note that both requests should reach the responser:

Request received: [Hello]
Request received: [Hello]

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.67%. Comparing base (eac2e69) to head (687edcc).
Report is 3 commits behind head on gz-transport13.

Additional details and impacted files
@@                Coverage Diff                 @@
##           gz-transport13     #477      +/-   ##
==================================================
- Coverage           87.69%   87.67%   -0.03%     
==================================================
  Files                  59       59              
  Lines                5704     5710       +6     
==================================================
+ Hits                 5002     5006       +4     
- Misses                702      704       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from issue_468 to gz-transport13 February 2, 2024 20:05
@azeey
Copy link
Contributor

azeey commented Feb 7, 2024

Would it be possible to detect that the response type is Empty instead of relying on the timeout to decide that the request is one way? I like the idea of making some of the arguments optional (see #475, #474, #473), but there is a lot of existing documentation that uses the --reptype gz.msgs.Empty and --timeout arguments. It would be nice if those commands didn't timeout as well.

@caguero
Copy link
Collaborator Author

caguero commented Mar 8, 2024

26d669b

Sounds good to me. Then, now both:

gz service -s /oneway --reqtype gz.msgs.StringMsg --reptype gz.msgs.Empty --req 'data: "Hello"' --timeout 10

and

gz service -s /oneway --reqtype gz.msgs.StringMsg --req 'data: "Hello"'

shoudn't generate timeouts.

@caguero
Copy link
Collaborator Author

caguero commented Mar 8, 2024

@j-rivero , do you know why the same job (Ubuntu CI / Ubuntu Jammy CI) can pass and fail at the same time? It seems that GZ_PATH and g_gzVersion is undefined.

@azeey
Copy link
Contributor

azeey commented Mar 11, 2024

There have been some changes in #481 that affect this PR. I suggest merging from gz-transport13.

@caguero
Copy link
Collaborator Author

caguero commented Mar 11, 2024

Good to go again!

Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just one comment about the timeout.

src/cmd/gz.hh Outdated

/// \brief External hook to execute 'gz service -r' from the command line.
/// \param[in] _service Service name.
/// \param[in] _reqType Message type used in the request.
/// \param[in] _repType Message type used in the response.
/// \param[in] _timeout The request will timeout after '_timeout' ms.
/// If -1 is used, the request will be one-way and _repType
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users might expect a timeout of -1 as "wait forever", which would be the opposite of this. I would say just checking if the message type is Empty is sufficient, but if you think we should have the timeout check as well, we should definitely update the help message of the CLI to mention this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine just checking the response type. I updated it in 687edcc.

Signed-off-by: Carlos Agüero <[email protected]>
@caguero caguero merged commit 3a5553a into gz-transport13 Mar 13, 2024
8 of 10 checks passed
@caguero caguero deleted the gz_service_oneway branch March 13, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Call one-way requester / responder from command line
2 participants