restify / clients

HttpClient, StringClient, and JsonClient extracted from restify
MIT License
57 stars 34 forks source link

Hitting request timeout throws an unhandled error #160

Closed pcwiek closed 6 years ago

pcwiek commented 6 years ago

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 https://github.com/restify/clients/blob/fbf5813fb4a7f24472985778fbd25640956f643c/lib/JsonClient.js#L70

Crashing version:

https://github.com/restify/clients/blob/dac7eb15bdc6758b1e1c9d8a0ad6e124421cb594/lib/JsonClient.js#L77

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.

DonutEspresso commented 6 years ago

Thanks @pcwiek ! This is definitely a bug - seems this may be a case where the timeout is occurring as it's trying to establish a connection (hence, no response object yet created). I've filed a PR.

nanandn commented 6 years ago

When will this fix be released?

DonutEspresso commented 6 years ago

Hi @nanandn, 2.0.1 has been published with this fix.

nanandn commented 6 years ago

@DonutEspresso Thanks for publishing. Will try it soon.