Adapter Method Promise Rejections are Unhandled, causes Unreliable Upstream Promise Resolutions

[willllhallll]

Describe the bug
Promises that reject with errors from within adapter methods are in some cases unhandled, leading to UnhandledPromiseRejectionWarning and potentially leaving pending upstream promises that never resolve.

An example of such an unhandled promise rejection is during the email sign-in flow. In this case, if the adapter method getUserByEmail rejects with an error, then an UnhandledPromiseRejectionWarning is logged to the node console.

In development, upstream promises (such as the one returned by the client signIn method when redirect is false) that rely on the getUserByEmail promise resolving or rejecting are left indefinitely pending.

In production, upstream promises are eventually rejected by virtue of a 500 response from the server as it deals with the unhandled rejection exception.

Steps to reproduce
Either clone repro:
https://github.com/WillTheVideoMan/nextauth-repro-unhandled-promise-rejection-warning

Or:

1. Configure next-auth in a development environment with an adapter.
2. To test this case, configure the adapter such that the adapter methods always reject with an error e.g. provide incorrect database credentials.
3. Make a request to an endpoint which contains an unhandled adapter promise e.g.
   

   next-auth/src/server/routes/signin.js

   Line 42 in 17b7898

   const profile = await getUserByEmail(email) || { email }

4. Observe an UnhandledPromiseRejectionWarning warning in the node console.
5. Observe that any upstream promises such as the one making the request are indefinitely pending.
6. Deploy instance to a production environment.
7. Observe that the upstream promises are resolved eventually by virtue of a 500 response.

Expected behavior
Expect all promises which may reject to be handled explicitly.

I think the overall intention here should be to avoid unhandled promise rejections entirely. This would ensure errors more consistently cascade in predictable ways, and importantly, would move in line with the upcoming node v15 revisions: nodejs/node#33021

Screenshots or error logs

(node:21664) UnhandledPromiseRejectionWarning: Error: get_user_by_email_error
    at getUserByEmail (webpack-internal:///./auth/fauna-adapter.js:135:31)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
(node:21664) 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(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 14)

Additional context
Whilst ultimately the production environment is the one to worry about, enabling consistent error handling throughout NextAuth would reduce the disparity between development and production. I discovered this issue exactly because of the lack of promise fulfilment in development, and so I had no way to properly test my implementation of error handling. This is a make-or-break for our testing methodology. It should be that we get the same fundamental behavior in development as we do in production, independent of any intrinsic NextJS magic.

Feedback

• Found the documentation helpful
• Found documentation but was incomplete
• Could not find relevant documentation
• Found the example project helpful
• Did not find the example project helpful

[balazsorban44]

I have some opinions on this matter, and ultimately I agree with you, and I think we could do a much better job at error handling.

Eg:

• Using throw instead return Promise.reject(new Error
• don't pass multiple parameters to Error in some places
• use custom errors at certain places (first good step was fix(errors): expose custom errors #1816)
• better document those errors (right now there are a lot of unexplained errors at https://next-auth.js.org/errors) Add missing error explanations to docs #1089
• don't use throw "string" as an option for redirecting at certain places, it's just weird and unconventional (eg.: you can throw "/some-url" in the signIn callback, if you want to stop the login flow. 😬signIn callback does nothing when returning an object #751 (comment), feat: allow to return string in signIn callback #1019)

It also touches on a subject that I think we shouldn't mix Promises with async/await code, because it gets harder to work with it, and causes confusion for our users. I've previously talked about this in other issues, #731, #891 are good examples.

The adapter code exceptionall stands out in this, and it would be awesome to improve, but I think they have to be improved in their new alternatives over at the other repo https://github.com/nextauthjs/adapters.

Right now the Prisma one looks the closest to what I think we should aim for:

https://github.com/nextauthjs/adapters/blob/canary/packages/prisma/src/index.ts

Personally, I haven't been in a codebase for a very long time where I had to rely on a single Promise. async/await should be how we consistently write our code.

My views on this might conflict with others on the team though, and that's why I haven't been doing much around this yet.

Getting feedback on this from the community helps to prove my point though, so thanks for opening this!

[kripod]

I’m really thankful to see this issue and the proposals of @balazsorban44. I couldn’t find out why NextAuth endpoints timed out and really glad to see this issue being explored in so much detail.

async functions are only intended to be used when an await call is inside the method body. A Promise could be returned even from a non-async function and then the returned value may be awaited on inside another async function.

Making functions async unnecessarily and using methods like Promise.reject – especially inside async functions – results in code that’s barely comprehensible and highly prone to errors.

Besides that, I also feel that it’s unnecessary to transpile async functions for modern node versions.

I would love to see reducing the usage of the async keyword in the entire project. Also, I recommend not to ever call methods like Promise.reject and Promise.resolve manually.

[balazsorban44]

https://github.com/typescript-eslint/typescript-eslint/blob/v4.22.0/packages/eslint-plugin/docs/rules/return-await.md Might be a good addition to this discussion

[kripod]

This has been causing a lot of server errors in production for our company lately. May we try refactoring the codebase to fix these issues for people using adapters?

[kripod]

The issue seems to happen every time a session expires, if that could help investigating the bug further.

[willllhallll]

Thank you for your responses! I ultimately agree that an async / await style gives the most readable and consistent codebase, and I really like the idea of exposing and throwing custom errors in certain places rather than using Promise.reject at your own peril. Moving away from verbosely controlling promise fulfilment feels like a sleek solution.

My only question is; does refactoring the return style of the adapters help with the unhandled awaits in the core? E.g.:

next-auth/src/server/routes/signin.js

Line 42 in 17b7898

const profile = await getUserByEmail(email) || { email }

Will profile resolve to something sensible? To me, it seems that the core codebase must still be refactored to surround all awaits that could throw in a try / catch block, as per the return-await ESLint rule you shared, or some other such valid method to handle any throws from within awaits. Either that, or as it appears the author intended with ||{email}, errors are not thrown within the adapter and instead false is returned from within the catch of the adapter method.

You will have to forgive me for my lack deep understanding on this matter. I hope some of this makes sense. I appreciate your time!

Cheers,
Will

[balazsorban44]

@WillTheVideoMan slightly off-topic, but I see you have a custom adapter for FaunaDB. we have a built-in one over at https://github.com/nextauthjs/adapters/tree/canary/packages/fauna feel free to contribute to that if you think it needs improvements! (We are working on better out-of-the-box TS support à la a better Adapter<ExpandWithGenerics> interface)

[willllhallll]

@balazsorban44 For sure, I have already made some small changes in collaboration with others and will certainly contribute them towards the adapter as soon as is possible. Looking forward to making Fauna a first-class adapter, and will use the new Prisma adapter for structural guidance.

[balazsorban44]

#1871 Here is my take. We could take away the responsibility from the adapters to implement proper error handling.

Currently just playing with this idea, I don't think it is there yet, feedback is welcome.

[willllhallll]

#1871 Looks and feels great, and will go a long way to help adapter authors to feel confident that error handling has been approached and implemented already. Awesome!

My only thought is that I am not sure how far this goes to solving the heart of the issue, which is the actual handling of errors from within the core itself. Whilst I think lifting the handling out of the adapter directly is a super idea, there are still instances within the core where a simple nullish comparator is used in places which could benefit from more explicit handling e.g.

next-auth/src/server/routes/signin.js

Line 42 in 17b7898

const profile = await getUserByEmail(email) || { email }

I have sketched out the following to try and illustrate my point. I have used your proposed new error handling methods here in combination with two 'core' functions which represent how the core might interact with adapters.

Note how the coreCodeNoHandle function will give non-descriptive 500s or console errors in browsers, whilst coreCodeTryCatch at least affords the opportunity to determine what happens next:

// Returns a promise which will reject on demand.
const databaseQuery = (reject) => {
  return new Promise(function (resolvePromise, rejectPromise) {
    if (reject) rejectPromise(new Error("Database Promise Reject"));
    else resolvePromise({ data: "useful" });
  });
};

// A private adapter method with no error handling.
const _adapterMethod = async () => {
  // Toggle `true` and `false` to control the dummy behaviour.
  const data = await databaseQuery(true);
  return data;
};

// An adapter method error handling wrapper funciton. *This idea is so great*! 
const adapterMethodErrorHandler = () => {
  return async function adapterMethod(...args) {
    try {
      return await _adapterMethod(...args);
    } catch (error) {
      throw new Error(error);
    }
  };
};

/**
 * Here, the core code uses the existing unwrapped `await` style where a
 * nullish comparison determines the value of `data`.
 *
 * However, when a database error is thrown, `data` never receives
 * its default shape.
 *
 * The error remains unhandled, we cannot describe what happens
 * to `data`.
 */
const coreCodeNoHandle = async () => {
  const adapterMethod = adapterMethodErrorHandler();
  const data = (await adapterMethod()) || { data: "default" };
  console.log(data);
};

/**
 * Here, the error-wrapped adapter method is further wrapped in a
 * `try/catch` directy within the core.
 *
 * Here, in the event of an error thrown, the value of
 * `data` can be explicitly set.
 *
 * We get to determine the value of data, and proceed accordingly.
 * Further, we can directly control what happens next e.g. API route
 * return shapes, redirects, etc...
 */
const coreCodeTryCatch = async () => {
  const adapterMethod = adapterMethodErrorHandler();
  let data;
  try {
    data = await adapterMethod();
  } catch (error) {
    data = { data: "default" };
  }
  console.log(data);
};

// Comment out the funcitons to see their differing styles.
coreCodeNoHandle();
//coreCodeTryCatch();

Maybe I am thinking too deeply on this, and such changes are not vital. To me, it could potentially allownext-auth package users to feel more confident that all errors are dealt with and presented to them in an understandable way e.g. through returning API route error objects of a known shape, or client-side error objects e.g. the signIn method. I would love to hear your thoughts.

[kripod]

@WillTheVideoMan I love that idea! Fallbacks could greatly improve the experience of end-users, although I think that issues like that should be logged as warnings via appOptions.logger.

[balazsorban44]

@WillTheVideoMan some progress, check #1871 and

nextauthjs/adapters#75

Available at next-auth@3.20.0-canary.3 @kripod will test it out in the next week

[willllhallll]

@balazsorban44 @kripod This is so great! I will pull down the latest canary release and see how things are working out.

I am incredibly grateful that you both are receptive and proactive to these kinds of issues, where elsewhere they may go unnoticed for months, or stall whilst awaiting some corporate approval.

This is a true open-source mindset, and can only be positive for the package and the NextJS ecosystem as a whole.

Thank you for your time and energy. It is greatly valued.

[balazsorban44]

This is now in next-auth@3.20.0! @WillTheVideoMan do you want to have a look at anything else before this issue is closed?

[balazsorban44]

Just wanted to give a heads up, I am backporting this error handling to the built-in typeorm and prisma legacy adapters!

Follow along

• TypeORM fix(adapter): upgrade legacy adapters #1952 and feat(typeorm-legacy): bring typeorm up-to-date adapters#77
• Prisma feat(prisma-legacy): bring Prisma legacy up-to-date adapters#78

[willllhallll]

@balazsorban44 All looks good to me! Glad the error handling is lifted up and out of the adapters, a pattern I love to see! I think this solves the unhandled exceptions issue I was facing, so from my perspective the issue can be closed.

Thanks again for your work.

[matha-io]

Hello everyone !

I feel like this issue is still here !
When trying to logging, an error is thrown in getUserByEmail from the adapter : TypeError: Cannot read property 'value' of undefined. With the new error system, adapterErrorHandler does the try / catch, but still throws the error :

https://github.com/nextauthjs/next-auth/blob/main/src/adapters/error-handler.js

export default function adapterErrorHandler(adapter, logger) {
  return Object.keys(adapter).reduce((acc, method) => {
    const name = capitalize(method)
    const code = upperSnake(name, adapter.displayName)

    const adapterMethod = adapter[method]
    acc[method] = async (...args) => {
      try {
        logger.debug(code, ...args)
        return await adapterMethod(...args)
      } catch (error) {
        logger.error(`${code}_ERROR`, error)
        const e = new UnknownError(error)
        e.name = `${name}Error`
        throw e
      }
    }
    return acc
  }, {})
}

Obviously, adapter methods can still throw errors, and I thought the whole point of this discussion was to handle them in the core. Using the wrapper helps log issues, but then the error is still thrown and I really don't know where to catch those errors. I tried to wrap the signIn() client side method in a try / catch, but it's not working. The /api/auth/signin/email is pending forever because of UnhandledPromiseRejectionWarning.

Do you have any idea how to catch those unhandled promises ? Thank you :)

[balazsorban44]

Could you collaborate more? Which adapter/version/next-auth is this happening in? Had a quick look of some of our adapters, none of them read a .value property, so curious.

[matha-io]

Hello @balazsorban44,

This comes from a custom Fauna Adapter, https://github.com/nextauthjs/adapters/blob/main/packages/fauna/src/index.js
The error thrown comes from a updatedAt: new Date(x.updatedAt.value),, because x.updatedAt can be null.
I know the Adapter should probably handle the case of the value being null, but the main reason of the question is what if there's an error with the DB call itself ? How would the error be handled ?

This is the function that throws :

async getUserByEmail(email) {
  if (!email) {
    return null;
  }

  const FQL = q.Let(
    {
      ref: q.Match(q.Index(indexes.User), email),
    },
    q.If(q.Exists(q.Var('ref')), q.Get(q.Var('ref')), null),
  );

  const { ref, data: user } =
    (await faunaClient.query<UserQueryResult>(FQL)) || {};

  if (!user) {
    return null;
  }

  const result = {
    ...user,
    id: ref.id,
    emailVerified: user.emailVerified
      ? new Date(user.emailVerified.value)
      : null,
    createdAt: new Date(user.createdAt.value),
    // updatedAt: new Date(user.updatedAt.value), this one is crashing
    updatedAt: user.updatedAt?.value
      ? new Date(user.updatedAt.value)
      : new Date(),
  };

  return result;
},

[balazsorban44]

Thanks for the extra info.

The adapter error wrapper doesn't actually stop errors from happening, it just wraps the methods so they get a more useful name to identify where the error happened. I guess it's still up to the adapter to make sure it doesn't throw an error, but we could wrap this piece of code so it returns the user to an error page as well:

next-auth/src/server/routes/signin.js

Line 50 in 75ca097

const profile = (await getUserByEmail(email)) || { email }

[matha-io]

Wrapping the getUserByEmail here would mean we need to wrap all others Adapter methods which I guess is not what we want. I will keep try / catch on the Adapter then.

I had an other question though that is still not answered : #2417
Would you mind taking a look ? 🙏

Thanks :p