Allow asynchronous service callbacks #258
Conversation
When advertising a service with roslibjs, the service callback returns a boolean value and fills out a response. Immediately upon returning, this response is sent back to the service callee. This model, while fine for quick service callbacks, prevents service callbacks that take time and must run asynchronously. As a result, a service callback could not (for example) call another service as part of it's computation before responding. This patch allows this, by using the Javascript Promise API. Service callbacks can now return either a boolean or a promise -- if a boolean, the service response is sent immediately (as before). If a promise, the service response is only sent when the promise resolves. This allows for asynchronous code to be used within service callbacks!
|
This patch is in reference to #257. |
|
Here is some HTML code to test with: <!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<script src="http://cdn.robotwebtools.org/EventEmitter2/current/eventemitter2.min.js"></script>
<script src="roslib.js"></script>
<script>
// Connecting to ROS
// -----------------
var ros = new ROSLIB.Ros();
// If there is an error on the backend, an 'error' emit will be emitted.
ros.on('error', function(error) {
document.getElementById('connecting').style.display = 'none';
document.getElementById('connected').style.display = 'none';
document.getElementById('closed').style.display = 'none';
document.getElementById('error').style.display = 'inline';
console.log(error);
});
// Find out exactly when we made a connection.
ros.on('connection', function() {
console.log('Connection made!');
document.getElementById('connecting').style.display = 'none';
document.getElementById('error').style.display = 'none';
document.getElementById('closed').style.display = 'none';
document.getElementById('connected').style.display = 'inline';
});
ros.on('close', function() {
console.log('Connection closed.');
document.getElementById('connecting').style.display = 'none';
document.getElementById('connected').style.display = 'none';
document.getElementById('closed').style.display = 'inline';
});
// Create a connection to the rosbridge WebSocket server.
ros.connect('ws://localhost:9090');
// Set up a sub service -- will be called inside another
// service call
var subservice = new ROSLIB.Service({
ros: ros,
name: '/add_two_ints',
serviceType: 'rospy_tutorials/AddTwoInts'
});
// The main service we advertise
var addTwoIntsService = new ROSLIB.Service({
ros: ros,
name: '/add_two_ints_web',
serviceType: 'rospy_tutorials/AddTwoInts'
});
addTwoIntsService.advertise(function(req, resp) {
console.log("Website received request to add two ints");
// Create (and return) a promise that will resolve only when the sub service responds
var promise = new Promise((resolve, reject) => {
// Call the sub service. Set up request as below
console.log("Calling another service to actually do the addition");
var sub_request = new ROSLIB.ServiceRequest({
a: req.a,
b: req.b
});
subservice.callService(sub_request, (result) => {
// Once we get a response, fill out resp and resolve
// the promise
console.log("Website received response from other service; web responding with its response")
resp.sum = result.sum
resolve(true); // Indicate service success
}, (failed) => {
console.log("Website received failure from other service; web responding with failure")
resolve(false); // Indicate failure
});
});
return promise;
});
</script>
</head>
<body>
<h1>Service Callback Testing</h1>
<p>This page advertises a ROS service, <tt>/add_two_ints_web</tt>, that calls another service (<tt>/add_two_ints</tt>) to actually do the addition.</p>
<p>To test, please run the following in the terminal:</p>
<ol>
<li><tt>roscore</tt></li>
<li><tt>rosrun rospy_tutorials add_two_ints_server</tt> (or even a slower version with a sleep!)</li>
<li><tt>roslaunch rosbridge_server rosbridge_websocket.launch</tt></li>
<li>(Refresh this page so it says connected below)</li>
<li><tt>rosservice call /add_two_ints_web "{a: 1, b: 2}"</tt></li>
</ol>
<div id="statusIndicator">
<p id="connecting">
Connecting to rosbridge...
</p>
<p id="connected" style="color:#00D600; display:none">
Connected
</p>
<p id="error" style="color:#FF0000; display:none">
Error in the backend!
</p>
<p id="closed" style="display:none">
Connection closed.
</p>
</div>
</body>
</html> |
|
To test, please run: This example features a service advertised by the website, that in turns calls another service to actually add two ints. Thank you very much! Please let me know if you have any questions. |
|
It will only work with the apps with Also, note that Do you have any other example that actually requires long time process? The given example seems to simple to understand your use case. |
|
Hi Jihoon, Thanks for your thoughts. Please let me clarify. My end goal is not to allow asynchronous code in a callback, or to try and handle multiple service requests asynchronously (I agree that rospy wouldn't handle that anyway). Rather, my goal is to call other services from within my service callbacks. In my opinion, this is a very important feature for any ROS library to have (it is possible with rospy, roscpp, roslisp, etc.), but to my knowledge it is not possible currently with roslibjs? Since Javascript doesn't allow "waits", returning a promise seems the natural way to handle this. As far as this patch requiring promise support, that is true. However, Promises are a built-in feature in Javascript and have been a few years now, so no external 3rd party library would be strictly required. Additionally, for those who do not want to use the Promise API, this patch allows service callbacks to return a boolean as before (no change). My use case is one where I would like to implement a service in Javascript that uses other services as part of its computation before returning a result. The add_two_ints example calling another add_two_ints is a toy example, but gets the point across. Thanks, I hope that clarifies. |
|
I need a second opinion. @RobotWebTools/maintainers how do you guys think? |
|
I also had a slight concern about the availability of Promises in some non-evergreen browsers. Then again at a short glance of the code, the impact of calling Architecture-wise, I didn't quite understand why this as a problem:
Why would this not be possible today? In the callback of the Promises solve the common problem of callback hell or if you have, say, many async operations after which you might want to execute some final operation, but even that I think can be overcome by some clever queuing tricks. If you are interested about the completion or success of the response before or after it resolved, I'm sure it can also be done with some flags wrapped in a self-executing function or within a custom "manager" object. Surely, this puts the burden of implementing and managing all this on the user, instead of the library providing this out of the box: so it's a matter of convenience, brainstorming and testing possible "real-world" scenarios. I think we should definitely consider implementing Promises support, but we shouldn't do it with just with Thanks for the contribution @sjlevine and for sparking the conversation! |
| // If the callback returned a boolean result, immediately | ||
| // send a response. | ||
| this._sendServiceResponse(rosbridgeRequest, response, result); | ||
| } else if (result.then) { |
There was a problem hiding this comment.
} else if (typeof result.then === 'function') {
| var self = this; | ||
| result.then(function(success) { | ||
| self._sendServiceResponse(rosbridgeRequest, response, success); | ||
| }); |
There was a problem hiding this comment.
remove self and bind this to the callback.
|
I agree with @viktorku's comments. Having a chain of service calls is not recommended process in ROS and it would be nicer to support Promises in more structured way if Promises is built-in library. But, this PR still improves a library usability without breaking a backward compatibility. So, I would like to merge this in. @sjlevine could you address @viktorku's reviews in the code? Then, it should be ready to merge. |
|
the travis build warns |
| // If the callback returned a boolean result, immediately | ||
| // send a response. | ||
| this._sendServiceResponse(rosbridgeRequest, response, result); | ||
| } else if (result.then) { |
There was a problem hiding this comment.
| // If a promise was returned, respond once it's resolved | ||
| var self = this; | ||
| result.then(function(success) { | ||
| self._sendServiceResponse(rosbridgeRequest, response, success); |
|
It is actually updated. This PR is just created before the updates. See #259. |
|
This PR has been inactive for a while. Please reopen if necessary. |
4 participants
When advertising a service with roslibjs, the service callback returns a
boolean value and fills out a response. Immediately upon returning, this
response is sent back to the service callee. This model, while fine for
quick service callbacks, prevents service callbacks that take time and
must run asynchronously. As a result, a service callback could not (for
example) call another service as part of it's computation before
responding.
This patch allows this, by using the Javascript Promise API. Service
callbacks can now return either a boolean or a promise -- if a boolean,
the service response is sent immediately (as before). If a promise, the
service response is only sent when the promise resolves. This allows for
asynchronous code to be used within service callbacks!