Node-restify: res.getHeaders() on node 8+ has infinite call stack

Created on 13 Jun 2017 · 12Comments · Source: restify/node-restify

Bug Report

Restify Version 4.3.0

Node.js Version 8.0.0/8.1.0

Expected behaviour

Calling res.getHeaders() would return an object of the headers in the response

Actual behaviour

RangeError: Maximum call stack size exceeded
    at ServerResponse.get (_http_outgoing.js:129:16)
    at ServerResponse.getHeaders (/usr/local/test-app/node_modules/restify/lib/response.js:177:17)
    at ServerResponse.get (_http_outgoing.js:130:17)
    at ServerResponse.getHeaders (/usr/local/test-app/node_modules/restify/lib/response.js:177:17)
    at ServerResponse.get (_http_outgoing.js:130:17)
    at ServerResponse.getHeaders (/usr/local/test-app/node_modules/restify/lib/response.js:177:17)
   ...

Repro case

app.get('/', (res, req, next) => {
    console.log(res.getHeaders()); //should not error out
    res.json({status: 'ok'});
    return next();
});

Cause

Node's ServerResponse inherits OutgoingMessage, which defines _headers as a function that calls this.getHeaders(). Within the restify response.js file, the getHeaders method returns this._headers, but since restify's response inherits the ServerResponse, this._headers, effectively calls this._headers.get, which calls this.getHeaders() and the cycle repeats.

Are you willing and able to fix this?

Yes, but wanted to make sure this wasn't already done and/or it was reproducible elsewhere.

Bug

Source

atyagi

👍8

Most helpful comment

@atyagi We found a solution which works for us. We'll see if all the tests pass and provide PR. We'll will reference this issue.

I think that they override Response.prototype.getHeaders

RobinMalfait on 15 Jun 2017

👍5

All 12 comments

We're seeing the same behaviour as of updating node V8. Any idea on how we can temp fix this?

kendepelchin on 15 Jun 2017

@atyagi do you want to keep us posted using this issue?

RobinMalfait on 15 Jun 2017

The use case that I had was around audit logging responses, which was solved by using res.getHeader() specifically for the headers that we needed. I didn't get a chance to dig deeper into the ServerResponse and OutgoingMessage objects unfortunately.

atyagi on 15 Jun 2017

@atyagi We found a solution which works for us. We'll see if all the tests pass and provide PR. We'll will reference this issue.

I think that they override Response.prototype.getHeaders

RobinMalfait on 15 Jun 2017

👍5

This issue was solved via #1369

retrohacker on 19 Jun 2017

Apologies for the radio silence on this bug. I was out of office for the past two weeks.

retrohacker on 19 Jun 2017

No problem @retrohacker . Glad the issue was found/fixed before me. Out of curiosity, would it be possible to pull the bugfix into a 4.x release? If you want, I can submit a PR to the 4.x branch.

atyagi on 21 Jun 2017

👍2

1378 :heart:

retrohacker on 23 Jun 2017

You guys are awesome. Thank you!

atyagi on 23 Jun 2017

@retrohacker @atyagi @RobinMalfait
The issue is not completed solved by PR https://github.com/restify/node-restify/pull/1378.
It still happens with node 8.0.0 and above

const restify = require('restify');
const server = restify.createServer();
server.get('/', function (req, res, next) {
  res.send(200, res.getHeaders());
  next();
});
server.listen(8080, function () {});

and a simple curl localhost:8080 will crash the server