next-auth: Adapter Method Promise Rejections are Unhandled, causes Unreliable Upstream Promise Resolutions

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 @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 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.

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 #1816)
• better document those errors (right now there are a lot of unexplained errors at https://next-auth.js.org/errors) #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. 😬https://github.com/nextauthjs/next-auth/issues/751#issuecomment-744472971, #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!

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 #1952 and https://github.com/nextauthjs/adapters/pull/77
• Prisma https://github.com/nextauthjs/adapters/pull/78

@WillTheVideoMan some progress, check #1871 and

https://github.com/nextauthjs/adapters/pull/75

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

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

@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.

#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.

https://github.com/nextauthjs/next-auth/blob/17b789822de64eb845e1e8e49ea83dbff56344f4/src/server/routes/signin.js#L42

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.

https://github.com/nextauthjs/next-auth/pull/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.

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.:

https://github.com/nextauthjs/next-auth/blob/17b789822de64eb845e1e8e49ea83dbff56344f4/src/server/routes/signin.js#L42

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

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.