fix: operation ends with next(null, null) gets 404

[osher]

bug fix, when an operation controller method ends with

    ctx.statusCode = 200
    next(null, null)

The response is yet 404 - because the mw does not understand the operation actually worked

[osher]

I still need to add a tests, will get to it later

[coveralls]

Coverage remained the same at 95.748% when pulling 7fe89659222eb9b4d15dade5c3ef5f1b8d74d056 on osher:patch-5 into a944eb97059da3f8fc26c614cbc39f8a9633b40f on theganyo:master.

[coveralls]

Coverage remained the same at 95.748% when pulling 437b6a2db873ceb0e8c1ffe7e075cf6c8af4c6ef on osher:patch-5 into a944eb97059da3f8fc26c614cbc39f8a9633b40f on theganyo:master.

[coveralls]

Coverage remained the same at 95.748% when pulling dc4ca91b224f3184d51c23975d9d7c684be2a031 on osher:patch-5 into a944eb97059da3f8fc26c614cbc39f8a9633b40f on theganyo:master.

[osher]

WTF!! this is wierd. look, we get specifically on node 8.x a stackoverflow error

snr~/lib/connect_middleware:169,26 tries:

169>        var headers = res._headers || res.headers || {};

But, on node 8, next line on the stack trace is: restify~/lib/response:177, which basically is:

176> Response.prototype.getHeaders = function getHeaders() {
177>    return (this._headers || {});
178> };
179> Response.prototype.headers = Response.prototype.getHeaders;

from that moment and on - it repeats in teh call-stack until it dies... :(

If I get it right - I suspect this._headers in node 8 for some reason executes this._headers - which recurses to death....

Is it what happens or is it just me? 😨

[osher]

Ah. no. there's also ServerResponse.get (_http_outgoing.js:115:17) which is a node-built-in in the loop. wtf

 at ServerResponse.getHeaders (C:\ws\open-source\swagger-node-runner\node_modules\restify\lib\response.js:177:17)
    at ServerResponse.get (_http_outgoing.js:115:17)
    at ServerResponse.getHeaders (C:\ws\open-source\swagger-node-runner\node_modules\restify\lib\response.js:177:17)
    at ServerResponse.get (_http_outgoing.js:115:17)
    at ServerResponse.getHeaders (C:\ws\open-source\swagger-node-runner\node_modules\restify\lib\response.js:177:17)
    at ServerResponse.get (_http_outgoing.js:115:17)
    at ServerResponse.hookEnd (C:\ws\open-source\swagger-node-runner\lib\connect_middleware.js:169:26)

[osher]

crap. look at that:

https://github.com/nodejs/node/blob/v8.x/lib/_http_outgoing.js#L113 which currently is:

Object.defineProperty(OutgoingMessage.prototype, '_headers', {
  get: function() {
    return this.getHeaders();
  },
  set: function(val) {
    if (val == null) {
      this[outHeadersKey] = null;
    } else if (typeof val === 'object') {
      const headers = this[outHeadersKey] = {};
      const keys = Object.keys(val);
      for (var i = 0; i < keys.length; ++i) {
        const name = keys[i];
        headers[name.toLowerCase()] = [name, val[name]];
      }
    }
  }
});

[osher]

https://github.com/nodejs/node/issues/15091

[osher]

https://github.com/restify/node-restify/issues/1470

[osher]

I think node 8 build fails because of restify. What do you think?

[theganyo]

Yes. There's clearly some kind of recursive condition in the restify stuff.

[osher]

what would you like to do? Perhaps we should not use res._headers? can you remember why it's used there?

I'm referring snr~/lib/connect_middleware:169,26 tries:

169>        var headers = res._headers || res.headers || {};

[theganyo]

Hoo boy... well, Restify was relying on _headers. And, after digging around, guess what? They found the problem as well: https://github.com/restify/node-restify/pull/1369/commits/ec90a25d6e8a6f4faa6310758e3fb37fe2145d77

This might be tricky to fix and have backward compatibility for Restify.

[osher]

if it wasn't tricky I wouldn't need to consult with you...

can you guide me how you want to proceed with this? mind that this error is not a result of my PR, we just discovered it.

Also mind that I modified .travis to include tests for node 6 in addition to latest (which moved from 6 to 8)

On Thu, Sep 7, 2017 at 5:31 PM, Scott Ganyo notifications@github.com wrote:

Hoo boy... well, Restify was relying on _headers. And, after digging around, guess what? They found the problem as well: restify/node-restify@ec90a25 https://github.com/restify/node-restify/commit/ec90a25d6e8a6f4faa6310758e3fb37fe2145d77

This might be tricky to fix and have backward compatibility for Restify.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/theganyo/swagger-node-runner/pull/109#issuecomment-327817659, or mute the thread https://github.com/notifications/unsubscribe-auth/AAxBHWWKFnR0jT_0UFcxeR16RBUY6B74ks5sf_5LgaJpZM4PHMnZ .

[theganyo]

I agree with removing the res._headers reference. I'm just not sure the tests will reveal the full ramifications of the change. That said, it needs to be fixed. So, I'm thinking: just do it, ensure the existing tests succeed, and cut next the release as an incompatible version (v0.8).

[osher]

Hmm. here's the latest findings.

If I bump devDependencies.restify :^5.2.0` (from^4.0.3`) - all tests pass on both node 6 and node 8.

But. Restify 5.x is tested on node > 4.x (https://github.com/restify/node-restify/blob/master/.travis.yml)

I'll try without removing 0.x from our travis - lets see how it fares 😉

[coveralls]

Coverage remained the same at 95.748% when pulling 6b0c56e53f1e01c69d43a0c493b1748c23688c96 on osher:patch-5 into a944eb97059da3f8fc26c614cbc39f8a9633b40f on theganyo:master.

[osher]

wha'da ya know - it passes!

Anyway - I resorted to boosting version because removing req._headers did not help. The test suite did catch it - Assertions for missing headers and for content-type errors held it in.

Looking deeper - req.headers returned a function - the same one enters recursion when being called.

So I followed up on the link from Restify, and decided to check for myself if any fixes gone public.

So here we are.

Last question - do you still want to boost our version, or do you consider boosting dependency compatible?

[theganyo]

Looks good. Thanks for all your work on this.

Yes, I think we need to do a release with a new minor version so any old Restify clients on old node versions don't just up and break without warning. Unfortunately, that means we'll need to rev all the engine-specific middleware pieces as well.

[osher]

I see. wonderful. I'll follow up on this

On Tue, Sep 12, 2017 at 7:38 PM, Scott Ganyo notifications@github.com wrote:

Merged #109 https://github.com/theganyo/swagger-node-runner/pull/109.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/theganyo/swagger-node-runner/pull/109#event-1245669207, or mute the thread https://github.com/notifications/unsubscribe-auth/AAxBHSxeMtGAw9YJYY6pCQBpS3Xm0vUmks5shrOSgaJpZM4PHMnZ .