Hello,

We wanted to ignore a bad SSL certificate temporarily and was wondering how to do this. Unfortunately the documentation did not cover this. I noticed you have the rejectUnauthorized: false flag in your test which does exactly what we need. Would it be possible to add this to the documentation?

https://github.com/restify/clients/blob/master/lib/JsonClient.js#L65-L70

Shouldn't this error with message that it failed to parse the JSON

Hi there,
I see the example that uses basic auth, but hoping for OAuth2.0 native support.

Hi there,

I've been trying to wrap my head around this for 3 hours now, thinking it was something I was doing wrong, however...

Am I right in thinking that the restify-clients is intended for server-to-server communication instead of client-to-server?

Sorry, I need coffee!

MOVED FROM: restify/node-restify#539

Using latest restify (2.6.1) when json client makes http DELETE request it sets content-type to application/octet-stream instead application/json.

Check out this gist:
https://gist.github.com/spikhoff/9229833

It would be nice to have an option to follow redirects if the original url has returned a 3xx status. What do you think?

Copied from restify/node-restify#1637

Node version 6.12.3
Restify version 7.1.0

Hi, we are attempting to run our application with FIPS enabled Node, as per a requirement that we have. When using restify, I noticed that the JSONClient appears to always add an MD5 hash to the headers here in the StringClient.js

`

StringClient.prototype.write = function write(options, body, callback) {

var self = this;
var normalizedBody = body;
var proto = StringClient.prototype;

if (normalizedBody !== null && typeof (normalizedBody) !== 'string') {
    normalizedBody = qs.stringify(normalizedBody);
}


function _write(data) {
    if (data) {
        var hash = crypto.createHash('md5');
        hash.update(data, 'utf8');
        options.headers['content-md5'] = hash.digest('base64');
    }

`

MD5 hashes are not allowed by FIPS, so I need to somehow disable this? If i comment out this MD5 header locally, the request works in FIPS-MODE without issue. The problem is while this code is here the FIPS module will not allow us to move past the point "crypto.createHash"

Do we need the hash with every request? Is there a way to disable this option? I'm not that familiar with restify, and could not find a way to do it.

If not, can we get a new feature added that will allow us to by-pass this piece of code with an option?

The module Dtrace should be optionalDependency as it is not supported on all linux and windows operating systems.
The restify project has already made it optionalDependency.

In #101 (acadfc4), the "dtrace-provider" dependency was moved into an "optionalDependency" field. The correct name for the field is "optionalDependencies". I'll submit a PR against master, but I'd also like to get this backported to a 1.5.3 release if possible.

Node v6 changed crypto hash.update() to default to 'utf8' encoding, prior to that (Node.js v4 and older) it used 'binary' encoding.

This can cause a problem when a Node v6 client talks to a Node v4 server (and vice versa), as the default md5 hash generated between these two node versions will be different, e.g.

$ node-6.9 -e "var crypto = require('crypto'); console.log(crypto.createHash('md5').update('¥').digest('base64'));"
B2JeFC5KxZYd5XRyZXqIwQ==
$ node-4.8 -e "var crypto = require('crypto'); console.log(crypto.createHash('md5').update('¥').digest('base64'));"
qzr4Vm3dINfvybMUq+kHVQ==

We at Joyent have hit this issue a few times in different components (and yes, we do have systems running with older Node versions - and ones that won't likely change anytime soon).

You can see current restify clients md5 hash handling here doesn't specify an encoding:

To resolve this issue, I think restify clients could be made to support both md5 hash styles ('binary' and 'utf8'). It can do this by generating both hash encoding types and then comparing that at least one of these hashes matches the content-md5 header.

I can't use restify-client module, cause typescript build failed with webpack.

ERROR in ./node_modules/restify-clients/lib/helpers/bunyan.js
Module not found: Error: Can't resolve 'node-uuid' in '/Users/insanehong/workspace/works/node_modules/restify-clients/lib/helpers'
``

This problem resoved 26f4f1bd9e. so I want to update new version ASAP 

From work on restify/node-restify#878 and #85. HTTP proxy support is in lib/HttpClient.js. Note that the request module does some whitelisting of req.headers for proxying. Restify-clients should look at doing the same... and finding refs to authoritative docs/RFCs on that.

Moved from restify/node-restify#1674.

• Used appropriate template for the issue type
• Searched both open and closed issues for duplicates of this issue
• Title adequately and concisely reflects the feature or the bug

Feature Request

Use Case

@hekike fixed support for Node.js >= v10.x in #175. That work highlighted the fact that some streams are used both in flowing and non-flowing mode in the current implementation.

This has historically been a common source of bugs and confusion. I believe making sure that streams use either flowing or non-flowing mode APIs would make the code more robust.

For instance, with the changes from the PR mentioned above, if emitResult was later changed to emit the 'result' event asynchronously (using e.g. setImmediate or process.nextTick), it would re-introduce the same broken behavior (the client would not read the stream).

Example API

The StringClient class should be reading from the stream using .read() instead of .on('data') to make changes such as the one mentioned above not introduce any regression.

We should also audit other streams implementations in the code base and make sure they're not using a mix of flowing and non-flowing mode APIs.

Are you willing and able to implement this?

Yes.

I ran into an issue recently where a client was stuck waiting on a host that had crashed and rebooted, but never ended the connection because there wasn't any TCP traffic. This situation could have been detected and remedied earlier by configuring the TCP connection to send keep-alive probes. I took a look, and Restify currently has an option _keep_alive which will enable keep-alive on the socket. It would be nice if there was a way to also set the initial delay (TCP_KEEPIDLE) in sending the probe (the second argument to .setKeepAlive()).

I don't know exactly what the interface should look like, but it could be something along the lines of:

{
    "keep_alive": {
        "initial_delay": <milliseconds>
    }
}

(Or perhaps just replace _keep_alive. Since _keep_alive doesn't seem to be currently documented, and it starts with an underbar, I'm guessing it's not considered a stable option?)

If node ever grows a way to also set TCP_KEEPCNT and TCP_KEEPINTVL, then additional fields could be added to the object for manipulating those parameters on the socket.

Adding this test to ensure <jsonclient>.post(...) without passing a body
argument works (as it does with node-restify.git/lib/clients/json_client.js).

diff --git a/test/index.js b/test/index.js
index 95579d0..e5ee8bf 100644
--- a/test/index.js
+++ b/test/index.js
@@ -356,6 +356,16 @@ describe('restify-client tests', function () {
         });
     });

+    it('POST json without body arg', function (done) {
+        JSON_CLIENT.post('/json/mcavage', function (err, req, res, obj) {
+            assert.ifError(err);
+            assert.ok(req);
+            assert.ok(res);
+            assert.deepEqual(obj, {hello: 'mcavage'});
+            done();
+        });
+    });
+

     it('PUT json', function (done) {
         var data = { hello: 'foo' };

This fails:

$ make test
...
  restify-client tests
...
    ✓ HEAD json
    ✓ POST json
    ✓ POST json empty body object
    1) POST json without body arg
    ✓ PUT json
    ✓ PATCH json
...


  134 passing (3s)
  1 failing

  1) restify-client tests POST json without body arg:

      AssertionError: body (object) is required
      + expected - actual


      at JsonClient.write (/Users/trentm/tm/restify-clients/lib/JsonClient.js:34:12)
      at JsonClient.post (/Users/trentm/tm/restify-clients/lib/StringClient.js:64:18)
      at Context.<anonymous> (/Users/trentm/tm/restify-clients/test/index.js:360:21)
      at callFnAsync (/Users/trentm/tm/restify-clients/node_modules/mocha/lib/runnable.js:349:8)
      at Test.Runnable.run (/Users/trentm/tm/restify-clients/node_modules/mocha/lib/runnable.js:301:7)
      at Runner.runTest (/Users/trentm/tm/restify-clients/node_modules/mocha/lib/runner.js:422:10)
      at /Users/trentm/tm/restify-clients/node_modules/mocha/lib/runner.js:528:12
      at next (/Users/trentm/tm/restify-clients/node_modules/mocha/lib/runner.js:342:14)
      at /Users/trentm/tm/restify-clients/node_modules/mocha/lib/runner.js:352:7
      at next (/Users/trentm/tm/restify-clients/node_modules/mocha/lib/runner.js:284:14)
      at Immediate._onImmediate (/Users/trentm/tm/restify-clients/node_modules/mocha/lib/runner.js:320:5)
      at processImmediate [as _immediateCallback] (timers.js:383:17)



make: *** [test] Error 1

The issue is this assert.object call:
https://github.com/trentm/clients/blob/7a90b56a04442fb1fadbceb602ff2a2f3e777177/lib/JsonClient.js#L34

In [email protected] the assert-plus dependency was updated from
0.1.5 to 1.0.0. One thing was missed in that update, from the assert-plus
changelog:

...
## 0.2.0

- Fix `assert.object(null)` so it throws
...

Notably node-restify.git#4.x still uses [email protected] so it doesn't hit
this issue.

We'd like to have one consistent style for all of the repos.

looking through the code, I found this: https://github.com/restify/clients/blob/master/lib/HttpClient.js#L722

Evidently doing this is deprecated:

var options = {
  path: '/foo/bar',
  query: {
    foo: 'bar',
    abc: ['xyz', '123']
  }
};

client.get(options, function(err, req, res) { .. });

What is the accepted way then?

I try to pass a JSON object, containing a array of objects, into restify and it contains an array, this is the result:

{
'alarms[0][sensor_id]': 't0',
'alarms[0][min]': '0',
'alarms[0][max]': '30',
'alarms[0][ignore]': 'false',
'alarms[1][sensor_id]': 't1',
'alarms[1][min]': '0',
'alarms[1][max]': '30',
'alarms[1][ignore]': 'false',
'alarms[2][sensor_id]': 't2',
'alarms[2][min]': '0',
'alarms[2][max]': '30',
'alarms[2][ignore]': 'false' }

i dont think this is correct

If consumers of the client pass a bare hostname for the url option to the client factory methods, the resulting client will unfortunately make requests to localhost.

This appears to be a result of lax checking of the output from the borderline byzantine url.parse() provided as part of the Node standard library. For example, if the value server.example.com is provided as url, the following is the parsed result:

{
    "protocol": null,
    "slashes": null,
    "auth": null,
    "host": null,
    "port": null,
    "hostname": null,
    "hash": null,
    "search": null,
    "query": null,
    "pathname": "server.example.com",
    "path": "server.example.com",
    "href": "server.example.com"
}

This is so wrong as to be actively dangerous. One need not think for a long time to imagine security issues that could result from convincing a poorly constructed application to make requests against itself.

I think we should probably check the value of protocol to make sure it is either "http:" or "https:". If not, we should likely throw.

StringClient directly decodes chunks to utf8 strings which is not safe in the case of a chunk boundary falling within a multibyte character. Instead it should be using the string_decoder api which accounts for this. https://nodejs.org/api/string_decoder.html

Moved from: restify/node-restify#742

The HTTP spec allows for DELETE request bodies, and the restify server works with DELETE request bodies, but the restify clients' del method doesn't allow a payload as an argument.

Note from maintainer

This is being migrated here from the restify/node-restify repo restify/node-restify#773

Original Issue Body

The documentation states that the JSON client can be initialized with an accept option that will set the value of the accept header. As far as I can tell, it is being ignored.

Here is my test case:

server

var restify = require('restify');

// initialize server
var server = restify.createServer({
  name    : 'restify-debug',
  version : '0.0.1',
});

server.get('/', function (req, res, next) {
  console.log(req.headers);
  res.send(200);
  return next();
});

// start the server
server.listen(1337, function () {
  console.log('%s listening at %s', server.name, server.url);
});

client

var restify = require('restify');

// initialize the JSON client
var client = restify.createJsonClient({
  url    : 'http://localhost:1337',
  accept : 'application/vnd.chrisallenlane.restify.v1.0.0+json',
});

client.get('/', process.exit);

The value of the accept header being logged to my console is "application/json".

Contrast that with curl:

curl http://localhost:1337 -H 'accept: foo'

The value of the accept header being logged to my console is "foo", which is what I'd expect. That indicates to me that the problem is with the client, and not the server.

I'll note that I tried setting the header value "manually" by initializing the JSON client with a headers property, but headers.accept was being ignored as well.

This is occurring with restify version 2.8.5.

Hi,

It looks like that http_client implementation does not currently support CRL's while using HTTPS. All other needed tls.secureContext options (e.g. ca, cert, key) are supported but from some reason crl option is not.

Is this a bug ? Would it be possible to add support also for crl option to http_client ?

Before restify-clients was split from restify, one would get access to Restify's Bunyan reserializers (especially its client_req and client_res serializers) via:

var restify = require('restify');
var log = bunyan.createLogger({
    ...
    serializers: restify.bunyan.serializers
});

This is required to create a Bunyan logger for restify clients so that you don't get big garbage in trace-level logs when client_req and/or client_res is logged (e.g. here:

There is some history and subtlety here:

• If you pass a Bunyan logger with no serializers to the restify clients, then it will add the necessarily serializers:

  clients/lib/HttpClient.js

  Lines 401 to 405 in 8765244

  	if (!this.log.serializers) {
  	// Ensure logger has a reasonable serializer for `client_res`
  	// and `client_req` logged in this module.
  	this.log = this.log.child({serializers: serializers});
  	}

  However that code is insufficient if your logger has other serializers on it.
• That limitation was due to: restify/node-restify#501

However, even if that case is fixed (I'll try to do a PR for that), I think it would be good citizenship for restify-clients to export .bunyan.serializers for users that want to control their Bunyan logger serializers carefully. I'd be careful to NOT export the RequestCaptureStream class that was carried over from restify (the server) itself because it isn't used or needed in the client.

I'm trying to use this repo from another project, but when I try to "npm install" that project, npm prompts for a password. I think it's because of this dependency in node-restify-clients:
https://github.com/restify/clients/blob/master/package.json#L66

I can reproduce this myself on my test system:

$ npm install git+https://[email protected]/restify/errors.git#master
\assword: -

and it just hangs there (with the spinny spinning). If I hit enter a few times, it actually works. But this is breaking an automated build environment.

This appears to be the problem described here:
http://stackoverflow.com/questions/19107160/why-is-npm-asking-for-a-password
npm/npm#3956

Currently, clients will take two components, a url, and a path. We should revisit this interface and standardize a common interface that makes it easy to specify a URL. Something like restify/node-restify#843.

Similar to server.requestLogger().

Similar to #104, it would be helpful if just about every request error included IP and port information so that we knew who we were talking to. Once #109 is landed, I'm happy to take a swing at this using that change as a model.

For context, we had a bug during network partitions where we ended up trying to use a client socket that the server had already closed, but because of the partition, the client didn't know that the server had closed it. All we got back from restify is "read ECONNRESET". We hit this again in a preproduction environment where we think there was a network partition limited to some components, and if we knew the IP addresses involved in these requests, we could have confirmed that.

We've also had issues where only some instances of a service were affected by a bug, and in that case it's really helpful if the client errors include the IPs of the instances they were talking to.

See: https://github.com/restify/clients/blob/master/lib/JsonClient.js#L65

If the response is malformed the parse SyntaxError should be returned as the first argument to the callback to be handled there.

Hi, maybe I am doing it wrong, but I am not able to set the CharSet in the headers.

(Needed to send some japanese body)
Usually in postman: Content-Type: application/json; charset=UTF-8

I`ve tried to set the headers :

{ 'Content-Type': 'application/json; charset=UTF-8' }

But it fails. It still send the wrong text.

MOVED FROM: restify/node-restify#642

HttpClient.close() reaches into the Node agent and calls end() on all of the sockets in use by the agent. It doesn't check that the socket's remote address matches the server that the client was created with. More importantly, though, the Node documentation says not to touch these sockets, and the problem with doing this is that if you call client.close() and then try to make another request (with a different client or even outside restify, AFAICT), the Node agent may decide to use one of the sockets that has started closing, and it will immediately get a "socket hang up" error.

I don't think there's a great way to implement close() given the current agent API, but what restify's doing right now makes it possible to run into bugs like node-manta#194. If the point of close() is basically for command-line programs to wrap things up, then it should probably be a global function (i.e., not per-agent), since it's closing everything, and it probably needs to be provided by the agent implementation.

• Node Version: 8.9.4
• Restify Version: 4.3.3

Hello.

I'm running an express-gateway locally based on this example: API Gateway: the Microservices Superglue.

Although I am using followRedirects: true with JsonClient, it doesn't seem to follow the redirects.

By way of a sanity check I can manually do the same thing withPostMan and get a response from the underlying service. If I then turn off 'automatically follow redirects, I will the same 301 that my code get's below.

// (omitted) perform a client credentials exchange to get access token for my api call
//...

var client = restify.createJsonClient({
	url: 'http://localhost:8080',
	followRedirects: true,
});

client.headers.authorization = `Bearer ${access_token}`;   
client.get('/api', (err, req, res, obj) => {
	assert.ifError(err);
	console.log('%d -> %j', res.statusCode, res.headers);
	console.log(obj);
});

The output is:

301 -> {
	"x-powered-by": "Express",
	"x-ratelimit-limit": "1000",
	"x-ratelimit-remaining": "999",
	"date": "Wed, 09 May 2018 02:45:19 GMT",
	"server": "Apache",
	"location": "http://uinames.com/api/",
	"content-length": "231",
	"content-type": "text/html; charset=iso-8859-1",
	"connection": "close"
}
{}

Trying to send a body that consists solely of a string causes JsonClient to throw because it asserts that the body must be an object.

But strings are valid JSON payload. They can be serialized to and deserialized from JSON transparently. The whole request passes just fine if I remove over-stringent assertion.

I believe the assertion should be relaxed to allow all valid JSON root types:string, boolean, number, array and object. (Did I forget something?)

Allow setting of headers and timeouts on a per request basis.

The node-uuid module is deprecated. A warning is displayed that the uuid module should be used instead.

Background: If we set requestTimeout in client options and it gets hit, we'll get an unhanded exception.

Version: 2.0.0
Last working version (that I checked): 1.5.0

Let's jump right into it:

TypeError: Cannot read property 'body' of null
    at parseResponse (/project/node_modules/restify-clients/lib/JsonClient.js:77:26)
    at ClientRequest.parseResponse (/project/node_modules/restify-clients/lib/StringClient.js:248:13)
    at Object.onceWrapper (events.js:317:30)
    at emitTwo (events.js:126:13)
    at ClientRequest.emit (events.js:214:7)
    at ClientRequest.wrapped (/project/node_modules/newrelic/lib/transaction/tracer/index.js:181:22)
    at ClientRequest.wrappedRequestEmit (/project/node_modules/newrelic/lib/instrumentation/core/http-outbound.js:50:26)
    at /project/node_modules/restify-clients/lib/HttpClient.js:244:29

And here's what I found:

Working version

clients/lib/JsonClient.js

Line 70 in fbf5813

obj = obj || {};

Crashing version:

The callback in StringClient in the error branch is called as callback(resErr, req, null, null), which means that access to res.body will always fail because it's always null.

If it's just a matter of changing the assignment from:

obj = obj || res.body || {};

into

obj = obj || (res && res.body) || {};

then I can do it, but I'm not sure if that's the whole story.

If there's a whitespace in the URL, parsing of the host and port numbers can fail and cause requests to default to 80. Whitespaces should probably be trimmed from the url argument when the client is created.

More of a question than an issue.

When I create a JsonClient in node I do the following:

var client = restify.createJsonClient({
  url: 'https://www.domain.com:4321/api'
});

Once I've done that, I make calls like so:

client.post('/service/path', { });

Which seems right. I expect that the path called would be something like https://www.domain.com:4321/api/service/path. However, what is happening is that the client is throwing away the /api base path and calling https://www.domain.com:4321/service/path.

I don't get it - I'm inserting the client URL into a config file, so that I can change hosts without any hassle; Now that I need a base path, I need to change the code as well as the config.

In certain scenarios like ECONNRESET, these methods are not yet set up and would throw:

    Error: socket hang up (code="ECONNRESET")
        at createHangUpError (_http_client.js:254:15)
        at Socket.socketOnEnd (_http_client.js:346:23)
        at emitNone (events.js:91:20)
        at Socket.emit (events.js:185:7)
        at endReadableNT (_stream_readable.js:974:12)
        at _combinedTickCallback (internal/process/next_tick.js:80:11)
        at process._tickDomainCallback (internal/process/next_tick.js:128:9)
Uncaught TypeError: req.getMetrics is not a function

We should either create noops for these, and or do best effort to capture what metrics/timings we can.

COPIED: restify/node-restify#483

The restify client doesn't retry socket hang-up errors that happen before a response is received. Here's a test program:

var mod_bunyan = require('bunyan');
var mod_fs = require('fs');
var mod_restify = require('restify');

var log, server, client, sockpath;

log = new mod_bunyan({
    'name': 'test',
    'level': process.env['LOG_LEVEL'] || 'info'
});

/* Clean up our UDS from an unclean shutdown. */
sockpath = '/tmp/mysock';
try {
        log.info('removing ', sockpath);
        mod_fs.unlinkSync(sockpath);
        log.info('removed ', sockpath);
} catch (ex) {
        if (ex['code'] != 'ENOENT')
                throw (ex);
        log.info('not found ', sockpath);
}

/*
 * Create a restify server that receives requests at /hello, waits 5 seconds,
 * and then closes the socket abruptly (before having sent any response).
 */
server = mod_restify.createServer();
server.get('/hello', function (req, res, next) {
        log.info('server: got request');
        req.socket.setTimeout(5000);

        /*
         * Close the client and server sockets 30 seconds after we receive the
         * first request so that our program exits.  This is just to demonstrate
         * that restify isn't actually retrying the request at this point.
         */
        setTimeout(function () {
                server.close();
                client.close();
        }, 30000);
});

/*
 * Start the server.  When it's ready, run the test case.
 */
server.listen(sockpath, function () {
        var rqoptions;

        log.info('server: listening');

        /*
         * Now that the server's up, create a restify client for the socket path
         * and submit a GET request to /hello.  We set the retry policy to 4
         * retries, but the restify client won't retry at all.
         */
        client = mod_restify.createStringClient({
            'socketPath': sockpath,
            'log': log,
            'version': '*'
        });

        rqoptions = {
            'path': '/hello',
            'retry': {
                'retries': 4,
                'minTimeout': 1000,
                'maxtimeout': 8000
            }
        };

        log.info('client: making request');
        client.get(rqoptions, function (err, req) {
                if (err) {
                        /*
                         * This is where the restify client reports an error --
                         * without having retried the request due to a socket
                         * hang-up.
                         */
                        log.error(err, 'client: error BEFORE request');
                        return;
                }

                log.info('client: request started');
                req.on('result', function (err2, res) {
                        if (err2) {
                                log.error(err2, 'client: error before response');
                                return;
                        }

                        log.info('client: response started');
                });
        });
});

Here's the output:

$ node -v
v0.8.26

$ time node test.js | bunyan
[2013-11-19T18:29:33.100Z]  INFO: test/34525 on bbc0462f: removing  /tmp/mysock
[2013-11-19T18:29:33.101Z]  INFO: test/34525 on bbc0462f: not found  /tmp/mysock
[2013-11-19T18:29:33.183Z]  INFO: test/34525 on bbc0462f: server: listening
[2013-11-19T18:29:33.202Z]  INFO: test/34525 on bbc0462f: client: making request
[2013-11-19T18:29:33.206Z]  INFO: test/34525 on bbc0462f: server: got request
[2013-11-19T18:29:38.208Z] ERROR: test/34525 on bbc0462f: client: error BEFORE request
    Error: socket hang up
        at createHangUpError (http.js:1379:15)
        at Socket.socketOnEnd (http.js:1476:23)
        at Pipe.onread (net.js:422:26)

real    0m30.312s
user    0m0.348s
sys     0m0.091s

Tried on restify 2.3.5, 2.6, and #master (9b07786060c240fcb1592289886d21d1f04c99c1).

This is a cross-post from my question on SO. I suspect this is a configuration mistake on my end, and would appreciate any help understanding where that mistake lies.

I am running several Restify v4.2.0 services in separate processes. One of them is special because it needs to be up and listening before all of the others even start.

Assume that I cannot use IPC, and all services must talk to the special service via HTTP. This leaves me with HTTP clients that attempt to contact the special service ad infinitum, waiting until it is up.

Hence this client with intentionally extreme options.

const client = restify.createStringClient({
    url,
    retry: {
        retries: Infinity,
    },
});

// Inside a promise
client.get(endpoint, function(err, req, res, data) {
    if (err) {
        reject(err);
    } else {        
        // ...

        // Probably not needed for GET, but here anyway.
        req.end();
        resolve(data);
    }
});

No matter how I configure the client, I still immediately get socket hang up errors. I want the nodeback to never fire until it finally reaches the service.

What do I need to do to my configuration to accomplish this?

I have a proposal for better JSONClient ergonomics. Currently, the client swallows potential JSON.parse errors, then creates an empty object and sets that as the parsed result:
https://github.com/restify/clients/blob/master/lib/JsonClient.js#L65

This is confusing and can be very misleading. JSONClient should do best effort parsing on incoming payloads. Seeing that the JSON RFC has been expanded to include strings as valid JSON, i.e., '"hello"', I propose the following:

1. If the client fails to parse the payload it should return a sensible parse error with useful information
2. If there is an HTTP error, that error trumps the parse error. Alternatively, we could return a MultiError - open to discussion on that.
3. If the client fails to parse the payload, it should return the original string payload instead of an empty object

These proposals would address #138 and #140 for more comprehensive JSON handling.

This breaks:

• restify/node-restify#729 - would argue that JSONP makes more sense in a separate JSONP client, given that JSONP is a completely different content type (application/javascript)
• restify/node-restify#203 - loose behavior that we are trying to close/address with this PR

Thoughts? cc @yunong @rajatkumar @retrohacker @hekike

sed -i 's/restify": "^4.3.0/restify": "^5.0.0/g' package.json && make prepush

  restify-client bunyan usage tests
    ✓ no logger (48ms)
    ✓ no serializers
    ✓ bunyan stdSerializers
    ✓ some unrelated bunyan serializer "foo"
    ✓ using restify-clients exported "bunyan.serializers"

  restify-client tests
TypeError: restify.acceptParser is not a function
    at Context.<anonymous> (/home/wblankenship/Development/restify-repro/1297/node_modules/restify-clients/test/index.js:152:32)
    at callFnAsync (/home/wblankenship/Development/restify-repro/1297/node_modules/restify-clients/node_modules/mocha/lib/runnable.js:371:21)
    at Hook.Runnable.run (/home/wblankenship/Development/restify-repro/1297/node_modules/restify-clients/node_modules/mocha/lib/runnable.js:318:7)
    at next (/home/wblankenship/Development/restify-repro/1297/node_modules/restify-clients/node_modules/mocha/lib/runner.js:309:10)
    at Immediate.<anonymous> (/home/wblankenship/Development/restify-repro/1297/node_modules/restify-clients/node_modules/mocha/lib/runner.js:339:5)
    at runCallback (timers.js:672:20)
    at tryOnImmediate (timers.js:645:5)
    at processImmediate [as _immediateCallback] (timers.js:617:5)
Makefile:82: recipe for target 'test' failed
make: *** [test] Error 1

For example, when we get ConnectionTimeoutErrors, all we get is:

ConnectTimeoutError: connect timeout after 4000ms

There are a couple of issues with this:

• people who see this don't know what we were connecting to (e.g., the IP or hostname and port)
• people who see this don't know what we were trying to do (e.g., what request this was)
• Based on rawRequest in lib/HttpClient.js, the problem isn't necessarily that we couldn't establish a connection, but rather that we couldn't obtain a socket within the timeout. The bulk of that time could have been spent doing a DNS lookup, queued behind other requests in the HTTP agent for the same host, or establishing the TCP connection.

What about something more like:

failed to complete HTTP request PUT /foo/bar to 10.1.2.3 port 1234: could not obtain connected socket within 4000ms

verror is intended to help build up the message in different layers of the stack.

This may sound excessively pedantic, but we run into the queueing and DNS issues kind of a lot and people burn a lot of time on the wrong path thinking that a connect timeout was a timeout attempting to establish a TCP connection.

Moved from: restify/node-restify#1265

The issue can be easily reproduced, all you need is just a simple HTTP server which replies with a delay (e.g. 10 sec). Here is a simple script to illustrate the problem

const restify = require('restify');
const assert = require('assert');
const exec = require('child_process').exec;

var server = restify.createServer();

var i = 0;

server.get('/', function (req, res, next) {
    if (i++) {
        setInterval(function() {
            res.send('bar');
            next();
        }, 10000);

        return;
    }

    res.send('foo');
    next();
});

server.listen(7777, function() {
    console.log('server started');
});

var client = restify.createStringClient({
    url: 'http://127.0.0.1:7777',
    connectTimeout: 3000,
    requestTimeout: 3000,
    version: '*',
    retry: false
});

console.log('request 1');

client.get('/', function(err, req, res, data)  {
    assert(!err);

    console.log('request 2', new Date());
    client.get('/', function(err, req, res, data) {
        console.log("done", err, new Date());

        console.log('request 3', new Date());
        client.get('/', function(err, req, res, data) {
            console.log("done", err, new Date());
        });
    });
});

The output:

request 1
server started
request 2 Fri Jan 13 2017 16:03:30 GMT+0000 (GMT)
done { [RequestTimeoutError: request timeout after 3000ms]
  message: 'request timeout after 3000ms',
  name: 'RequestTimeoutError' } Fri Jan 13 2017 16:03:33 GMT+0000 (GMT)
request 3 Fri Jan 13 2017 16:03:33 GMT+0000 (GMT)
done null Fri Jan 13 2017 16:03:43 GMT+0000 (GMT)

As you can see third request took 10 seconds despite of requestTimeout.

As I understand the problem is here (requestTimeout is just not set if socket is not connecting):
https://github.com/restify/node-restify/blob/4.x/lib/clients/http_client.js#L223

Also, there is one more weird thing, for second request it works correctly because _socket._connecting == true, but my understanding is _socket._connecting should be false for the second request as well.

  1 failing

  1) restify-client tests GH-115 GET path with spaces:
     AssertionError: expected 'Request path contains unescaped characters' to equal 'Request path contains unescaped characters.'
      at Context.<anonymous> (/Users/trentm/src/restify-clients/test/index.js:302:20)
      at callFnAsync (/Users/trentm/src/restify-clients/node_modules/mocha/lib/runnable.js:349:8)
      at Test.Runnable.run (/Users/trentm/src/restify-clients/node_modules/mocha/lib/runnable.js:301:7)
      at Runner.runTest (/Users/trentm/src/restify-clients/node_modules/mocha/lib/runner.js:422:10)
      at /Users/trentm/src/restify-clients/node_modules/mocha/lib/runner.js:528:12
      at next (/Users/trentm/src/restify-clients/node_modules/mocha/lib/runner.js:342:14)
      at /Users/trentm/src/restify-clients/node_modules/mocha/lib/runner.js:352:7
      at next (/Users/trentm/src/restify-clients/node_modules/mocha/lib/runner.js:284:14)
      at Immediate._onImmediate (/Users/trentm/src/restify-clients/node_modules/mocha/lib/runner.js:320:5)
      at tryOnImmediate (timers.js:543:15)
      at processImmediate [as _immediateCallback] (timers.js:523:5)

This was broken by nodejs/node#3374

Restify gets stuck on a response with status code 204 and 'Content-Lenght' header valued higher than 0, eventually times out and executes the callback function.

Example server:

const http = require('http');

const server = http.createServer((req, res) => {
    const body = 'I\'m gonna go ahead and get you stuck for a bit...';
    console.log(body);
    res.writeHead(204, {
        'Content-Length': Buffer.byteLength(body),
        'Content-Type': 'text/plain',
    });
    res.end(body);
});

server.listen(8000, '127.0.0.1', () => {
    console.log(`Server running ...`);
});

Example client:

const restify = require('restify');

const client = restify.createJsonClient({
    url: 'http://localhost:8000'
});

const start = new Date().getTime();
client.del('/', () => {
    console.log('Aaand I\'m done... Took me: ', ((new Date().getTime() - start)/1000), 's');
});

We heavily use restify as the server with the audit on... That way, we monitor the entire Req/Res stack that comes to the server.

However, we started using the restify clients and we'd like to have the same functionality with the clients. Looking at the repository, I don't see that.

Please provide that implementation... Thanks!

restify-errors already creates the TooManyRedirectsError constructor. When attempted to be created for a second time, an assertion error is thrown:

https://github.com/restify/clients/blob/master/lib/HttpClient.js#L90