Unhandled Promise rejection when using ctx.res.send() in async afterRemote hook

[henkosch]

Description/Steps to reproduce

Look at the following example model:

module.exports = function (Test) {
    Test.solve = async function () {
        return 42;
    };

    Test.afterRemote('solve', async function (ctx, output) {
        ctx.res.setHeader('Content-Type', 'text/plain');
        ctx.res.send("Result: " + ctx.result);
    });

    Test.remoteMethod("solve", {
        http: { path: '/solve', verb: "get" },
        returns: { type: "number", root: true }
    });
};

I wanted to generate a plain text response for an endpoint. I could not find any other solution than to use an afterRemote hook to change the response. So I used ctx.res.send() to set the response explicitly inside the hook.

I'm using ES6 async functions everywhere where loopback supports it. These functions return promises so I can use await anywhere inside them which is very handy. So returning a promise can be used instead of calling next().

In the above example I get the following warning:

UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Can't set headers after they are sent.

I tried to find the cause of this behavior, so I converted the afterRemote hook back to the callback version like this:

Test.afterRemote('solve', function (ctx, output, next) {
    ctx.res.setHeader('Content-Type', 'text/plain');
    ctx.res.send("Result: " + ctx.result);
    next();
});

But then I get the following warning:

UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Callback was already called.

So I went further and removed any promises, and reverted my code back to the callback style:

Test.solve = function (next) {
    next(null, 42);
};

Test.afterRemote('solve', function (ctx, output, next) {
    ctx.res.setHeader('Content-Type', 'text/plain');
    ctx.res.send("Result: " + ctx.result);
    next();
});

Test.remoteMethod("solve", {
    http: { path: '/solve', verb: "get" },
    returns: { type: "number", root: true }
});

This crashed the nodejs process with the following error:

D:\dev\lbtest\node_modules\async\dist\async.js:903
        if (fn === null) throw new Error("Callback was already called.");
                         ^

Error: Callback was already called.
    at D:\dev\lbtest\node_modules\async\dist\async.js:903:32
    at D:\dev\lbtest\node_modules\async\dist\async.js:3858:13
    at D:\dev\lbtest\node_modules\strong-remoting\lib\remote-objects.js:717:7
    at execStack (D:\dev\lbtest\node_modules\strong-remoting\lib\remote-objects.js:522:7)
    at RemoteObjects.execHooks (D:\dev\lbtest\node_modules\strong-remoting\lib\remote-objects.js:526:10)
    at interceptInvocationErrors (D:\dev\lbtest\node_modules\strong-remoting\lib\remote-objects.js:716:10)
    at D:\dev\lbtest\node_modules\strong-remoting\node_modules\loopback-phase\node_modules\async\lib\async.js:148:21
    at D:\dev\lbtest\node_modules\strong-remoting\node_modules\loopback-phase\node_modules\async\lib\async.js:148:21
    at D:\dev\lbtest\node_modules\strong-remoting\node_modules\loopback-phase\node_modules\async\lib\async.js:148:21
    at D:\dev\lbtest\node_modules\strong-remoting\lib\remote-objects.js:679:7
    at D:\dev\lbtest\node_modules\strong-remoting\lib\http-context.js:303:16
    at SharedMethod.invoke (D:\dev\lbtest\node_modules\strong-remoting\lib\shared-method.js:286:12)
    at HttpContext.invoke (D:\dev\lbtest\node_modules\strong-remoting\lib\http-context.js:297:12)
    at phaseInvoke (D:\dev\lbtest\node_modules\strong-remoting\lib\remote-objects.js:677:9)
    at runHandler (D:\dev\lbtest\node_modules\strong-remoting\node_modules\loopback-phase\lib\phase.js:135:5)
    at iterate (D:\dev\lbtest\node_modules\strong-remoting\node_modules\loopback-phase\node_modules\async\lib\async.js:146:13)

So it seems the ctx.res.send() somehow calls the next() callback and closes the middleware pipeline. But this behavior interferes with the promise that is returned by the afterRemote hook, so it will be rejected, but the rejection is not handled.

Expected result

The example at the top should work, and should not produce an unhandled promise rejection warning, because it is deprecated and will crash the nodejs process in the future.

And since it's supported to return a promise inside an afterRemote hook, it should not be a problem to use ctx.res.send() inside an async hook.

Additional information

I am also interested in better ways to send text/plain response with a remoteMethod without an afterRemote hook.

[henkosch]

I just found that this is related to: #406

[bajtos]

Hello @henkosch, thank you for reporting the issue and sorry for the silence. Have you tried to use callback-style function for your afterRemote hook and do not call next?

Test.afterRemote('solve', function (ctx, output, next) {
    ctx.res.setHeader('Content-Type', 'text/plain');
    ctx.res.send("Result: " + ctx.result);
});

This is how Express middleware works - if the response was sent, then next is not called.

I do realise my proposal is hacky, it also means that no other afterRemote hooks registered for this method will be called after the response has been sent. Unfortunately we don't have bandwidth now to take a closer look at the issue.

[bajtos]

send text/plain response with a remoteMethod without an afterRemote hook.

I think you should be able to use file type for this feature, see #284:

Allow users to specify { type: 'file', root: true } for the single argument that will be sent directly as the response body.

The following values are supported for file the file argument:

• String
• Buffer
• ReadableStream (anything that exposes .pipe() method)

The pull request description contains an example showing how to set Content-Type header from such remote method.

[gijo-varghese]

@bajtos any fix for this?

[bajtos]

@gijo-varghese A fix for what exactly?

[gijo-varghese]

@bajtos I need to use async like this:

Test.afterRemote('solve', async function (ctx, output) {
        ctx.res.setHeader('Content-Type', 'text/plain');
        ctx.res.send("Result: " + ctx.result);
    });

instead of

Test.afterRemote('solve', function (ctx, output, next) {
    ctx.res.setHeader('Content-Type', 'text/plain');
    ctx.res.send("Result: " + ctx.result);
});

[bajtos]

@gijo-varghese I see. I am afraid it's not possible to use async (or Promise-based) functions for remote hooks if you want to circumvent the default request processing.

Here are few things you can try:

Wrap your async function in callback-based hook handler.

async function onAfterRemote(ctx, output) {
  ctx.res.setHeader('Content-Type', 'text/plain');
  ctx.res.send("Result: " + ctx.result);
}
  
function noop() {}

Test.afterRemote('solve', function (ctx, output, next) {
  onAfterRemote().then(
    noop, // don't call next()
    next // pass any errors to next
  );
});

Inside your async hook handler, change ctx.result to undefined. IIRC, strong-remoting is not going to send any response in such case (assuming the response was already sent and/or the remote method is described as having no return arguments). I am not entirely sure about this, you will have to give it a try yourself.

Test.afterRemote('solve', async function(ctx, output) {
  ctx.res.setHeader('Content-Type', 'text/plain');
  ctx.res.send("Result: " + ctx.result);
  ctx.result = undefined;
});

[gijo-varghese]

ok. Thanks. Let me try