getResource() should take a callback and an optional timeout and error function

[cxbrooks]

Note: the issue was created automatically with bugzilla2github tool

Original bug ID: BZ#22
From: @cxbrooks
Reported version: unspecified
CC: [email protected]

[cxbrooks]

I'm going through email and to do items and creating bugs.

Below is an email thread about getResource().

On 7/2/17 8:23 AM, Edward A. Lee wrote:

The accessors spec defines a getResource() function that appears to only
be implemented in the Nashorn host (and CapeCode). But this function has
an incorrect API, I think. It takes a URI and a timeout as arguments and
returns a string. It should take a URI and a callback function as arguments.

I couldn't find any uses of it anywhere.

Any objections if I change it?

On Jul 7, 2017, at 11:48 AM, Christopher Brooks wrote:

The version of getResource() that takes a timeout argument
is used in many places.

$PTII/adm/bin/ptIItxtfiles >& /tmp/f
cat /tmp/f | grep .js | xargs grep 'getResource('
Here are the files to check:
bash-3.2$ cat /tmp/f | grep .js | xargs grep 'getResource('
awk -F : '{print $1}' | sort | uniq

./org/terraswarm/accessor/accessors/web/hosts/browser/browser.js

./org/terraswarm/accessor/accessors/web/hosts/common/commonHost.js
./org/terraswarm/accessor/accessors/web/hosts/jxcore/jxcore/app.js
/org/terraswarm/accessor/accessors/web/hosts/jxcore/jxcore/common/commonHost.js
./org/terraswarm/accessor/accessors/web/image/ImageAnnotate.js
./org/terraswarm/accessor/accessors/web/services/GeoCoder.js
./org/terraswarm/accessor/accessors/web/services/Heartbeat.js
./org/terraswarm/accessor/accessors/web/services/Weather.js
./org/terraswarm/accessor/accessors/web/test/TestFunctions.js
./ptolemy/actor/lib/jjs/JavaScript.java
./ptolemy/actor/lib/jjs/RestrictedJavaScriptInterface.java
./ptolemy/actor/lib/jjs/commonHost.js
/ptolemy/actor/lib/jjs/localFunctions.js

./ptolemy/actor/lib/jjs/modules/httpClient/test/auto/GeoCoderWeather.xml
./ptolemy/actor/lib/jjs/nashornHost.js

I agree that a callback version would be better than a
imeout version if the callback version can be used everywhere
the timeout version is used.

_Christopher

On Fri, Jul 7, 2017 at 9:17 PM, Edward A. LEE wrote:

The timeout argument is ignored, so even this version doesn't
make sense...

On Aug 13, 2017, at 2:26 AM, Marten Lohstroh wrote:

Have we ended up settling on a blocking or async getResource
function? The wiki still says:
*
*
getResource(uri, timeout)

I guess no change was made after all? Is there a use case for the
timeout? If so, do we want an async function that observes a
timeout instead?

On Sat, Aug 12, 2017 at 10:38 PM Edward A. LEE <eal@ berkeley.edu
<mailto:eal@ berkeley.edu>> wrote:

I think this design should be replaced with an async version.
I've been meaning to do that.
The current implementation ignores the timeout argument

On 8/13/17 9:49 AM, Marten Lohstroh wrote:

Do we want async with or without a timeout? I guess we can make the
timeout optional and ignore it like we have been doing so far

On 8/13/17, Edward wrote:

I think the usual pattern is to provide an optional timeout and an error
handler function.

[cxbrooks]

The current documentation for getResource is at
https://www.icyphy.org/accessors/wiki/Version1/Top-LevelJavaScriptFunctions#getResource

4.  Getting Resources

getResource(uri, timeout): Get a resource, which may be a relative file name or a URL, and return the value of the resource as a string. Implementations of this function will likely restrict the locations from which resources can be retrieved. A recommended policy for swarmlet hosts is to at least permit http and https accesses. Local files may be allowed, if for example they are given as relative file names relative to be in the same directory where the swarmlet model is located or in a subdirectory. 

https://docs.nodejitsu.com/articles/errors/what-are-the-error-conventions/ says:

-> In node.js, it is considered standard practice to handle errors in asynchronous functions by returning them as the first argument to the current function's callback. If there is an error, the first parameter is passed an Error object with all the details. Otherwise, the first parameter is null.

You can see this in https://nodejs.org/api/fs.html#fs_fs_readfile_path_options_callback

--start--
fs.readFile(path[, options], callback)

History

path <string> | <Buffer> | <URL> | <integer> filename or file descriptor
options <Object> | <string>
    encoding <string> | <null> Default: null
    flag <string> Default: 'r'
callback <Function>

Asynchronously reads the entire contents of a file. Example:

fs.readFile('/etc/passwd', (err, data) => {
if (err) throw err;
console.log(data);
});

The callback is passed two arguments (err, data), where data is the contents of the file.

If no encoding is specified, then the raw buffer is returned.

If options is a string, then it specifies the encoding. Example:

fs.readFile('/etc/passwd', 'utf8', callback);
--end--

Adding another toplevel function to the system is tedious, but we also want to keep backward compatibility. Currently, the default is to return a string. It also seems that the timeout argument is ignored.

We could redefine the function to be

getresource(path, options, callback)

path <string> | <Buffer> | <URL> | <integer> filename or file descriptor
options <Object> | <string>
    encoding <string> | <null> Default: null
    flag <string> Default: 'r'
    timeout <number> Default: 3000
callback <Function>

where the first argument of the callback are:
error
data