A recent checkin (d1366f2f860d0c1804fe74db1774eecf482d3ebf) stopped Restify's auto handling of OPTIONS requests. We now have the ability to create them manually.

Although more control is good, manually adding OPTIONS for every endpoint is cumbersome and (if your application relies on CORS requests) can make it hard to move to Restify v2.

I propose that the auto generated OPTIONS responses are "on by default" and developers can override the default by specifying server.opts(...). It would probably also be good to have a way to globally disable them.

Hi, I'd like to use node-restify in my project, but simple benchmark shows it's about 4x slower than raw node server. I had about 4k rps for restify and 12-13k for raw node (I used cluster to utilze all cpu cores). I know 4k is not bad, but I need very high throughput api. Are there any options in restify I could turn off to get better perf ?(I've turned off md5 hashing, but don't know what else I could turn off)

In Restify 6 one could pull up a named templated URI and replace the params with values to construct a URI. This was great for hypermedia APIs that create links to other resources.

This code was deleted in 7 (see https://github.com/restify/node-restify/commit/82832771826321480e5e524db258668f62b689c2) and I've looked everywhere for it's replacement.

Hints?

I recently migrated to restify beta, and following that restify.CORS is no longer available. I tried to use the restify-cors-middleware module but it doesn't work, when I looked closely at the npm list of installed modules I noticed it's not even supposed to work for restify beta? (beta is version 5 and above)

npm ERR! peer dep missing: [email protected] - 4.3.x, required by [email protected]

If that is the case, is there another way for me to use CORS in the beta?

Edit / resolution

The documentation about error handling is incomplete and confusing. Maybe it would help to explicitly clarify the following points in the section on error handling:

1. Handle your errors immediately, where they're generated. next(err) really means res.send(err). It's not an alternative to throw err. If you need to log and shut down, pass it off to an error handling module instead of stuffing it in next(err) where it will be swallowed.
2. server.on('uncaughtException'... is how you handle any errors that were thrown in middleware / routes. It works fine for that purpose, as long as you remember the first guideline. 'uncaughtException' will not be triggered for next(err) calls.
3. formatters are the way to customize error messages going out to the users. I don't think they should be used for the other tasks described in this thread. They're definitely not a viable alternative to dealing with errors in uncaughtException, because presumably, once you get there, you're sending the user the error you want them to see... not necessarily the error that was originally generated. See point 1.
4. For errors unrelated to middleware, remember to use process.on('uncaughtException'...

Here's the original issue, for posterity:

Problem

Restify swallows all errors coming from route handlers. It does invoke its own error handler, but that handler never bails, regardless of whether or not an error is recoverable.

Expected

It should be up to the application developer to determine whether or not a service restart is needed. While it should be safe to swallow 4xx errors, other types of unhandled errors could leave the application in undefined state, which could lead to all sorts of trouble.

Reproduce

Restify v2.6.0 Node v0.10.20

var restify = require('restify'),
  app = restify.createServer(),
  errorHandler = require('../error-handler.js'),

  handleError = function handleError(err) {
    console.log('Caught error!'); // never happens
    setTimeout(function () {
      process.exit(1);
    }, 3000);
  },

  middlewareError =
      function middlewareError(req, res, next) {
    throw new Error('Random middleware error.');
  };


app.get('/err', function (req, res, next) {
  // This doesn't get caught.
  next( new Error('Random unrecoverable error. ' +
    'Server is now running in undefined state!') );
});

app.get('/throwerr', function (req, res, next) {
  // This doesn't get caught.
  throw new Error('Random unrecoverable error. ' +
    'Server is now running in undefined state!');
});

// This gets caught, yay!
app.use(middlewareError);

app.get('/middleware', function (req, res, next) {
  // Placeholder to invoke middlewareError.
});

// This works for middleware. Fails for routes.
app.on('uncaughtException', handleError);

// Nope. This doesn't help.
process.on('uncaughtException', handleError);

app.listen(3000, function () {
  console.log('Listening on port 3000');
});

Hello,

i was trying my application that is using restify with nodejs v4.0.0, and it is not working. Once the require to restify is made the process just dies without giving any exception or error.

Thank you, Joao Franco

Discussion: async/await basic API

What?

Add support to async functions as middleware (.pre() and .use()) and route handlers. Keeps the first hook synchronous.

Example

const restify = require('restify');

const server = restify.createServer({});

server.use(async (req, res) => {
  req.something = await doSomethingAsync();
});

server.get('/params', async (req, res) => {
  const value = await asyncOperation(req.something);
   res.send(value);
});

Middleware API

Restify implementation uses Chains for both .pre() and .use() middleware, which allows a consistent experience. All middleware right now must call next() or next(err). The proposed interface for the middleware function is:

• fn.length === 2 (arity 2);
• fn instanceof AsyncFunction;
• If the async function resolves, it calls next();
• the value resolved will be discarded;
• if it rejects we call next(err) [error handling discussed in a section below];

Route handler API

Restify also uses Chains for both route handlers, which allows being very consistent. Right now, Restify treats route handlers just like middleware and forces the user to call next().

• fn.length === 2 (arity 2);
• fn instanceof AsyncFunction;
• If the async function resolves, it calls next();
• the value resolved will be discarded;
• user must call res.send to send the body;
• if it rejects we call next(err) [error handling discussed in a section below];

However, we also have the opportunity, if we want to, to improve DevEx and allow the value resolved of the promise to be sent as the body. It causes some challenges in Fastify, but there are mitigations we can apply.

For instance, we can check, inside the resolve handler for the async function, if the response has been flushed and if not we flush the response with whatever value the user returns. This approach has the drawback that if the user uses both, whatever he returns from the promise will be discarded.

Besides that, there is the question of what should happen if the user calls:

server.get('/params', async (req, res) => {
  setImmediate(() => {
      res.send(value);
  });
});

Should we wait for res.send and risk it never be called? Or should immediately call next() on resolution and let the timers being slightly off?

I propose we deal with this particular use case with documentation and user education.

If you have a callback context you should use a callback and if you have a promise or async/await context you should use the async handler.

Alternatively, we could add a new API to Response .sent which is a getter for a promise that resolves when the response is sent. This allows us to use a similar approach to Fastify:

server.get('/params', async (req, res) => {
  setImmediate(() => {
      res.send(value);
  });
  await res.sent;
});

Error handling

As of right now, Restify allows users to throw non-errors and wraps it around an InternalError after emitting the error events.

I propose that we keep the same behavior for the async handler, going through the same code path. The only exception is when the user returns the async function with a rejected promise without any value. For example:

server.get('/params', async (req, res) => {
  return Promise.reject();
});

We can't prevent users from doing that. That is a valid Javascript code, even though it is not very useful. What we can do it if the rejection value is falsy we can create an AsyncHandlerRejection VError, which will allow the user a better debugging experience.

PR

#1833

I am pretty sure I found a major memory leak. It seems to be related to the gzipResponse and bodyParser plugins. I use node-memwatch to inspect the memory usage and force a garbage collection.

Version Info

ubuntu: 12.04
node: 0.10.4
restify: 2.6.0

Server

var fs = require('fs');
var memwatch = require('memwatch');
var restify = require('restify');

var hd = null;

function startDiff(req, res, next) {
    memwatch.gc();
    hd = new memwatch.HeapDiff();
    return next();
}

function endDiff(req, res, next) {
    memwatch.gc();
    var diff = hd.end();

    console.log('before: ' + diff.before.size);
    console.log('after: ' + diff.after.size);
    console.log('change: ' + diff.change.size);
    console.log('---------------------');

    return next();
}

var server = restify.createServer();

// removing either plugin also exhibits the memory leak
// remove both plugins and no memory leak
server.use(restify.gzipResponse());
server.use(restify.bodyParser());

server.get('/', [
    startDiff,
    function(req, res, next) {
        // large-file.json is a just a large JSON file, content does not matter
        var data = fs.readFileSync('large-file.json'));
        res.send(JSON.parse(data));
        return next();
    },
    endDiff,
]);

server.listen(4001, function() {
    console.log('listening at ' + server.url);
});

Client

curl --compressed http://localhost:4001 > /dev/null

Output after 3 client calls, with memory leak

before: 5.99 mb
after: 175.48 mb
change: 169.5 mb
---------------------
before: 175.83 mb
after: 318.21 mb
change: 142.38 mb
---------------------
before: 318.4 mb
after: 460.75 mb
change: 142.35 mb
---------------------

Output after 3 client calls, with no memory leak

before: 5.87 mb
after: 147.82 mb
change: 141.94 mb
---------------------
before: 5.36 mb
after: 147.86 mb
change: 142.5 mb
---------------------
before: 5.4 mb
after: 147.88 mb
change: 142.48 mb
---------------------

I received the error message:

405 (Method Not Allowed)

when making an OPTIONS request. Backbone does this by default.

There seems to be lots of discussion about it in issue #284, but I didn't find a solution that worked with node-restify 2.8.2.

The code I am using is:

server.pre(restify.CORS());
server.use(restify.fullResponse());

What is the current recommended solution?

I want to be able to have a function that is called unhandled Promise rejections that happen inside a Restify middleware, to send the error to user. There is server.on('uncaughtException', ...) that catches all the errors, but if my middleware is async function or it returns a Promise, this handler won't catch it.

Example:

// server creation is omitted

server.on('uncaughtException', (req, res, route, err) => {
 console.log(err);
  res.send({ success: false, error: err.message });
});

server.get('/', async (res, req, next) => {
  throw new Error('Some error');
})

If I run this example, it won't call the error handler and will print this instead:

(node:23037) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Some error

Restify 4.3.0, tested with Node.js 6.10.0 (compiled with Babel) and 7.9.0 (it supports async natively)

Hey,

I have been stupid at some point - either now or when I suggested pull request #33. I have been playing around with my custom content writer and the only way I seem to be able to call it within the scope of the response object (for access to _code etc) is to explictly do (at around line 85 of http_extra.js):

else if (this._config.contentWriters[this._accept]) {
      var fn = this._config.contentWriters[this._accept];
      data = fn.call(this, _opts.body); //use .call() to force the scope
    }

Other than doing this, I seem to be having no luck due to all the references and mayhem with scopes that are going on elsewhere.

If you agree that this is the way to go then I will ping you a pull request (and even make sure its got semi-colons where needed!)

Cheers, Ben

Pre-Submission Checklist

• [x] Opened an issue discussing these changes before opening the PR
• [x] Ran the linter and tests via make prepush
• [x] Included comprehensive and convincing tests for changes

Issues

Closes:

• Issue #1935

Changes

Fix unstopped chain-handler when response already destroyed.

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

Restify Version: 10.0.0 Node.js Version: 16.8.1

Expected behaviour

Given a handler that does async work, I should be able to call next(false); and have the chain stop processing there.

Actual behaviour

The handler arity checks prevent me from having handlers that use next and are async

Repro case

Code similar to this is used in one of our projects using restify v8. It breaks when trying to update to v10:

server.use(async (req, res, next) => {
  const result = await someAsyncWork();
  if (shouldStop(result)) {
    res.send({something: 'here'});
    next(false);
    return;
  }

  // ... more work
  next();
});

I am aware I could make my handler synchronous, then do someAsyncWork().then(result => {...}) but async/await syntax was chosen for cleanliness and readability.

Cause

https://github.com/restify/node-restify/blob/2053ef6a7e16d380a4e33d40059ea987c7373e4c/lib/chain.js#L77-L101

Are you willing and able to fix this?

This probably requires reworking the async chain stuff, so no.

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

Restify Version: 10.0.0 Node.js Version: v18.12.1

Expected behaviour

Should be able to install restify as a dependency without generating npm warnings about deprecated formidable dependency.

Actual behaviour

npm install emits: npm WARN deprecated [email protected]: Please upgrade to latest, formidable@v2 or formidable@v3! Check these notes: https://bit.ly/2ZEqIau

Repro case

Add restify as a dependency to your package.json and run npm install.

Cause

Need to upgrade formidable dependency to latest v2 or v3.

Are you willing and able to fix this?

Yes

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

Restify Version: 9.0.0 Node.js Version: any

Expected behaviour

That the "engines" field of "package.json" accurately says what versions of node.js restify works with.

Actual behaviour

The "engines" for [email protected], and in the current repo tip, says "node": ">=10.0.0". However, with anything less that node v14.8.0 importing restify crashes:

% node --version
v14.17.6

% git remote -v
origin	[email protected]:restify/node-restify.git (fetch)
origin	[email protected]:restify/node-restify.git (push)
% git log --oneline -1
426f7e9 (HEAD -> master, origin/master, origin/HEAD) Update example to allow downgrading to http1

% node -e 'require("./")'
internal/modules/cjs/loader.js:892
  throw err;
  ^

Error: Cannot find module 'node:process'
Require stack:
- /Users/trentm/src/node-restify/lib/request.js
- /Users/trentm/src/node-restify/lib/server.js
- /Users/trentm/src/node-restify/lib/index.js
- /Users/trentm/src/node-restify/[eval]
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:889:15)
    at Function.Module._load (internal/modules/cjs/loader.js:745:27)
    at Module.require (internal/modules/cjs/loader.js:961:19)
    at require (internal/modules/cjs/helpers.js:92:18)
    at Object.<anonymous> (/Users/trentm/src/node-restify/lib/request.js:5:25)
    at Module._compile (internal/modules/cjs/loader.js:1072:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1101:10)
    at Module.load (internal/modules/cjs/loader.js:937:32)
    at Function.Module._load (internal/modules/cjs/loader.js:778:12)
    at Module.require (internal/modules/cjs/loader.js:961:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/Users/trentm/src/node-restify/lib/request.js',
    '/Users/trentm/src/node-restify/lib/server.js',
    '/Users/trentm/src/node-restify/lib/index.js',
    '/Users/trentm/src/node-restify/[eval]'
  ]
}

Repro case

(Shown above.)

Cause

The issue is the usage of "Core modules" (i.e. the node:-prefix) in "lib/request.js":

const { emitWarning } = require('node:process');

Are you willing and able to fix this?

Yes. A few questions, though:

• IIUC, The intent of commit c15111fb2862705d49dbd6cf60612069f13adb8d was to drop node v10 and v12 support, so would a PR to update "engines" be acceptable?

• Support for all of node v14 (back to v14.0.0) could be restored with the following change.

diff --git a/lib/request.js b/lib/request.js
index a4c54d0..d67dc96 100644
--- a/lib/request.js
+++ b/lib/request.js
@@ -2,7 +2,7 @@

 'use strict';

-const { emitWarning } = require('node:process');
+const { emitWarning } = require('process');

or even just this change:

-const { emitWarning } = require('node:process');
+const { emitWarning } = process;

Is that desired? I don't have a strong preference.

• It seems that the "9.x" branch is out of sync. Should I make my PR against the "master" branch, and ignore "9.x"?

PR https://github.com/restify/node-restify/pull/1923 is somewhat related to this. Thanks.

Prototype pollution vulnerability in function jsonBodyParser in jsonBodyParser.js in restify node-restify 9.0.0-rc.1 via the k variable in jsonBodyParser.js.

The prototype pollution vulnerability can be mitigated with several best practices described here: [https://learn.snyk.io/lessons/prototype-pollution/javascript/]