Sendgrid-nodejs: Unexpected promise rejection while using callback API

Created on 15 Jan 2019 · 14Comments · Source: sendgrid/sendgrid-nodejs

Issue Summary

When calling the mail send(..) with a callback, errors are passed to the callback — as expected — and a promise rejection is also produced — which is unexpected and undesirable, as the promise rejection escapes unhandled (other than the unhandled promise rejection handler, by which time it's too late).

Steps to reproduce

The code:

const sendgrid = require("@sendgrid/mail");
sendgrid.setApiKey(API_KEY);
sendgrid.send({
    attachments: [],
    subject: "testing sendgrid",
    html: "<html><head></head><body></body></html>"
}, function(error) {
    if (error) {
        console.error(error.message);
    }
});

Produces the following:

(node:87110) UnhandledPromiseRejectionWarning: Error: Provide at least one of to, cc or bcc
    at Mail.addTo (/Users/pc/sendgrid/node_modules/@sendgrid/helpers/classes/mail.js:274
:13)
    at Mail.fromData (/Users/pc/sendgrid/node_modules/@sendgrid/helpers/classes/mail.js:
112:12)
    at new Mail (/Users/pc/sendgrid/node_modules/@sendgrid/helpers/classes/mail.js:43:12
)
    at Function.create (/Users/pc/sendgrid/node_modules/@sendgrid/helpers/classes/mail.j
s:615:12)
    at MailService.send (/Users/pc/sendgrid/node_modules/@sendgrid/mail/src/classes/mail
-service.js:96:25)
    at Object.<anonymous> (/Users/pc/sendgrid/sendgrid.js:8:10)
    at Module._compile (internal/modules/cjs/loader.js:721:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:732:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
(node:87110) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejectionid: 1)

Looking at the code in /packages/mail/src/classes/mail-service.js:

    catch (error) {

      //Pass to callback if provided
      if (cb) {
        // eslint-disable-next-line callback-return
        cb(error, null);
      }

      //Reject promise
      return Promise.reject(error);
    }

From the eslint comment, it appears that the callback is intentionally not returned, therefore returning a promise rejection — but this clearly results in an unhandled promise rejection in code that is callback-oriented.

Technical details:

sendgrid-nodejs Version: @sendgrid/mail, 6.3.0
Node.js Version: 11.6.0

medium help wanted help wanted community enhancement up for grabs up-for-grabs

Source

pcrispim

👍1

Most helpful comment

Thanks for the feedback @adamreisnz. I'll leave this issue open and re-tag appropriately.

@Berkmann18, thanks for the continued support!

@pcrispim, thanks for your patience and feedback!

thinkingserious on 23 Jan 2019

👍2

All 14 comments

That's because send() is asynchronous so I guess Node didn't treat the error handling in the callback as the rejection handler.
Have you tried using the thenable version or the ES8 approach?

Berkmann18 on 17 Jan 2019

True, but the asynchronous nature of send() can be handled by callbacks or promises — however when using the callback form of send() a promise-like behaviour is unexpected.

For example, in the client.request() the result and error are issued correctly by returning a promise if there's no callback and invoking the callback function if there's one:

This way a promise- and a callback-driven forms of the API coexist without conflict — it's in the case of send() when an error is caught that this flow is broken because the error that was expected to be passed to the callback (exclusively) is also returned as promise rejection outside of a promise chain.

For the time being we must support the callback version, but it's not a problem in the long term as we're moving to an async-await version: promise rejection is caught naturally as part of the promise chain.

That said, I seems to be a bug and can still affect others... the solution is to return the callback (as recommended by the disabled eslint warning).

pcrispim on 17 Jan 2019

For the time being we must support the callback version, but it's not a problem in the long term as we're moving to an async-await version: promise rejection is caught naturally as part of the promise chain.

Well, you could still try to use ES6 with promises without having to use ES8's async/await version.
That would depend on the Node/NPM version you use/target and the level of support of ES6-8.

Ping me in 3h if I don't reply back with what I get using your code.

Berkmann18 on 17 Jan 2019

👍1

@adamreisnz,

What do you think about returning the callback to avoid this issue?

thinkingserious on 17 Jan 2019

I guess we can place a return on line 139, it will then return a promise that always resolves with the value of the callback, even when rejected.

adamreisnz on 18 Jan 2019

👍2

@pcrispim I just tried your code (I couldn't do it yesterday as my home laptop has a hard disk / GRUB problem yet to be resolved) and here's what I got:

Provide at least one of to, cc or bcc
(node:2183) UnhandledPromiseRejectionWarning: Error: Provide at least one of to, cc or bcc
  at Mail.addTo (/mnt/c/Users/max/Projects/Sandbox/Sendgrid/node_modules/@sendgrid/helpers/classes/mail.js:274:13)
  at Mail.fromData (/mnt/c/Users/max/Projects/Sandbox/Sendgrid/node_modules/@sendgrid/helpers/classes/mail.js:112:12)
  at new Mail (/mnt/c/Users/max/Projects/Sandbox/Sendgrid/node_modules/@sendgrid/helpers/classes/mail.js:43:12)
  at Function.create (/mnt/c/Users/max/Projects/Sandbox/Sendgrid/node_modules/@sendgrid/helpers/classes/mail.js:615:12)
  at MailService.send (/mnt/c/Users/max/Projects/Sandbox/Sendgrid/node_modules/@sendgrid/mail/src/classes/mail-service.js:96:25)
  at Object.<anonymous> (/mnt/c/Users/max/Projects/Sandbox/Sendgrid/index.js:3:10)
  at Module._compile (internal/modules/cjs/loader.js:707:30)
  at Object.Module._extensions..js (internal/modules/cjs/loader.js:718:10)
  at Module.load (internal/modules/cjs/loader.js:605:32)
  at tryModuleLoad (internal/modules/cjs/loader.js:544:12)
(node:2183) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:2183) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

I then tried using the same code but specifying a from and a to value (which, you should always do, hence the first line of your error) and I got no output (the mail was sent and received as expected):

const sendgrid = require('@sendgrid/mail');
sendgrid.setApiKey(process.env.SENDGRID_API_KEY);
sendgrid.send({
    from: '[email protected]',
    to: 'your.email...', //replace that with your email
    attachments: [],
    subject: 'testing sendgrid',
    html: '<html><head></head><body></body></html>'
}, function(error) {
    if (error) {
        console.error(error.message);
    }
});

Berkmann18 on 18 Jan 2019

@Berkmann18 yes, that's the cause of the error (the code generating the emails must validate the input before sending) — but it's still the case that the error reporting of send() when used with callbacks will produce an unhandled promise rejection... ;)

pcrispim on 18 Jan 2019

@pcrispim True (at least until someone does what @adamreisnz suggested, which I think is the right thing to do).

Berkmann18 on 18 Jan 2019

👍1

Thanks for the feedback @adamreisnz. I'll leave this issue open and re-tag appropriately.

@Berkmann18, thanks for the continued support!

@pcrispim, thanks for your patience and feedback!

thinkingserious on 23 Jan 2019

👍2

I have to disagree that this is a community enhancement. It's a pretty obvious defect which, given Node's warning that "[i]n the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code", verges on becoming a security issue.

Also, this seems to be a trivial fix, but I do not wish to get involved, as doing so would mean signing a CLA. Is there anyone actively supporting this project, who could fix this?

juona on 5 Mar 2019

I'm happy to open a pr for it

On Wed, Mar 6, 2019, 11:13 juona notifications@github.com wrote:

I have to disagree that this is a community enhancement. It's a pretty
obvious defect which, given Node's warning that "[i]n the future, promise
rejections that are not handled will terminate the Node.js process with a
non-zero exit code", verges on becoming a security issue.

Also, this seems to be a trivial fix, but I do not wish to get involved,
as doing so would mean signing a CLA. Is there anyone actively supporting
this project, who could fix this?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/sendgrid/sendgrid-nodejs/issues/872#issuecomment-469879315,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAd8QiuxMu7K0spHATqLLtHOHXEwBvNWks5vTuwdgaJpZM4aBDMl
.

adamreisnz on 6 Mar 2019

I could do that.

Berkmann18 on 6 Mar 2019

Thanks. What about publishing? Last publish is from 9 months ago, so even if this is fixed, I suspect it won't be released.

juona on 6 Mar 2019

I see two (potential) issues here:

If there is a client-side issue (e.g, missing to), the callback is executed _and_ the error is thrown (this is what the OP reported)
- MailService should return after executing cb(error, null); instead of returning a rejected promise
If there is a server-side issue (e.g., not authorized), the callback is executed with the error _and_ the returned Promise is a rejection
- #900 resolves this _if_ it's actually a problem