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

Register call_service operation to run in its own thread #847

Merged
merged 2 commits into from
Mar 8, 2023

Conversation

sea-bass
Copy link
Contributor

@sea-bass sea-bass commented Mar 3, 2023

Public API Changes
None

Description
I have changed the operation registered for call_service so that it's not blocking -- that is, the CallService.call_service() implementation now runs in its own separate thread.

This enables us to process multiple service calls in parallel, whereas right now the tools do them sequentially.

If folks are apprehensive about this blanket change, I could also envision allowing both options, so we could have a call_service_blocking operation that works as before, and a call_service that uses the changes from this PR... or some other way to configure things. Open to feedback!

Fixes #825

@sea-bass
Copy link
Contributor Author

sea-bass commented Mar 6, 2023

@achim-k what do you think of this change?

I'm particularly interested in: Should it go in as-is, or should we have some configurability on whether to have service calls block vs. run in their own threads?

Copy link
Contributor

@achim-k achim-k left a comment

Choose a reason for hiding this comment

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

@achim-k what do you think of this change?

I'm particularly interested in: Should it go in as-is, or should we have some configurability on whether to have service calls block vs. run in their own threads?

Looks like a good improvement and changes are minimal 👍

Do you expect that change to break any existing use case?
I think it would make sense to have some sort of configurability (maybe a parameter) so users can disable the new behavior if, for whatever reason, this causes problems.

@sea-bass
Copy link
Contributor Author

sea-bass commented Mar 6, 2023

Do you expect that change to break any existing use case?

I thought about this and there are two things that come to mind.

  1. If the call_service implementation actually returned something... which it doesn't. So we're safe here.
  2. If there is any kind of "post-execution" callback where some code is run at the end of the call_service operation, then yes, it might break things. Because this operation now ends when the thread is started, and not when the service actually returns a response.

Anyways, I will work on making this configurable. Should this be an extra arg in the call_service_msg_fields, or simply a separate operation to register? Like call_service vs. call_service_thread?

@achim-k
Copy link
Contributor

achim-k commented Mar 6, 2023

Should this be an extra arg in the call_service_msg_fields, or simply a separate operation to register? Like call_service vs. call_service_thread?

Both options would require making changes to the protocol specification and then adding support for this to the various client libraries (roslibjs, ...). How about just making this a server configuration for now?

E.g.

if call_services_in_thread:
  protocol.register_operation("call_service", lambda msg: Thread(target=self.call_service, ...
else:
  protocol.register_operation("call_service", self.call_service)

@sea-bass
Copy link
Contributor Author

sea-bass commented Mar 8, 2023

@achim-k What do you think of this update? Added a call_services_in_new_thread node parameter that can be configured at launch time. Defaults to False so it's not breaking.

@achim-k
Copy link
Contributor

achim-k commented Mar 8, 2023

@achim-k What do you think of this update? Added a call_services_in_new_thread node parameter that can be configured at launch time. Defaults to False so it's not breaking.

Looks good, nice improvement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service calls on same websocket are executed sequentially, not simultaneously
2 participants