3
\$\begingroup\$

Below is a (very) simplified version of a function I made that creates multiple REST requests to gather needed information.

Things to note:

  • One of the REST calls is dependent upon the output of another.
  • I want to send off multiple requests at the same time when possible
  • If any rest responses inform us that the user is logged out, null should be returned.
async function gatherUserInfo(userId) {
  try {
    // Don't await yet, so this request can be sent at the same time as future requests.
    const groupsRequest = getGroups(userId)

    const {username, name} = await getProfile(userId)
    return {
      username,
      name,
      statusText: await getStatusText(username),
      groups: await groupsRequest,
    }
  } catch (err) {
    if (err instanceof NotLoggedInError) {
      return null
    }
    throw err
  }
}

I found the above code to be a very elegant solution to the problem. However, I recently realized that it won't actually work in node, because node will give warning or stop execution (depending on version) if an uncaught error happens in a promise. In the example above, the getGroups() call would send off a request (but not await it). Then, this function execution will pause on getProfile(). In the meantime, getGroups() will come back with the NotLoggedInError, which will result in the process being terminated, because that promise has not been awaited yet, (the try-catch can't catch the error until the await happens, which does happen later in the function execution).

So, I reluctantly trashed my old design and coded this up instead:

async function gatherUserInfo(userId) {
  const requests = [];

  requests.push(
    getGroups(userId)
    .then(groups => ({groups}))
  )

  requests.push(
    getProfile(userId)
    .then(async ({username, name}) => {
      const statusText = await getStatusText(username)
      return {username, name, statusText}
    })
  )

  let responses
  try {
    responses = await Promise.all(requests)
  } catch (err) {
    if (err instanceof NotLoggedInError) {
      return null
    }
    throw err
  }

  return Object.assign({}, ...responses)
}

This is much harder to follow or understand, but it's the best I could come up with. How can this code be improved?

Here's some stub methods and test cases that can be paired with the examples above to execute them:

// Mocks //

async function getGroups(userid) {
  await wait(50)
  if (userid !== 12) throw new NotLoggedInError('User not logged in')
  return ['group A', 'group B'];
}

async function getProfile(userid) {
  await wait(30)
  if (userid !== 12) throw new NotLoggedInError('User not logged in')
  return { username: 'grinchMaster135', name: 'The Grinch' };
}

async function getStatusText(username) {
  await wait(30)
  return 'away'
}

class NotLoggedInError extends Error {}

const wait = ms => new Promise(resolve => setTimeout(resolve, ms))


// TEST //

;(async function() {
  console.log('Grinch info:', await gatherUserInfo(12)) // Grinch info: { username: 'grinchMaster135', name: 'The Grinch', groups: [ 'group A', 'group B' ], statusText: 'away' }
  console.log('Non-logged in user info:', await gatherUserInfo(13)) // Non-logged in user info: null
})()
```
\$\endgroup\$
5
\$\begingroup\$

Your code boils down to just executing getGroups() in parallel with the getProfile()-getStatusText() sequence.

You might want to extract that sequence out to a separate function. This way, it's obvious that they're treated as one thing (two async calls build one object). You can then Promise.all() the return of this new function (getStatus() in the following example) and getGroups(). This way, you also make it obvious that the two calls are async and independent of each other.

// getProfile and getStatusText synchronous to each other.
const getStatus = async id => {
  const {username, name} = await getProfile(id)
  const statusText = await getStatusText(username)
  return { username, name, statusText }
}

const gatherUserInfo = async userId => {
  try {

    // getStatus and getGroups will run asynchronous to each other.
    const [{ username, name, statusText }, groups] = await Promise.all([
      getStatus(userId),
      getGroups(userId)
    ])

    return { username, name, statusText, groups }

  } catch(e) {
    if (e instanceof NotLoggedInError) {
      return null
    }
    throw err
  }
}
\$\endgroup\$
5
\$\begingroup\$

When you have multiple asynchronous requests to make, and you want them to operate in parallel, usually the appropriate tool to use is Promise.all. You can use it here to wait for the groups and the profile data at once, despite the fact that the two requests are pretty distinct from each other:

async function gatherUserInfo(userId) {
  try {
    const [groups, {username, name}] = await Promise.all([
      getGroups(userId),
      getProfile(userId),
    ])
    return {
      username,
      name,
      groups,
      statusText: await getStatusText(username),
    }
  } catch (err) {
    if (err instanceof NotLoggedInError) {
      return null
    }
    throw err
  }
}

This way, if any one of the Promises rejects, it will be being awaited at that time, so it'll be caught properly.

async function gatherUserInfo(userId) {
  try {
    const [groups, {username, name}] = await Promise.all([
      getGroups(userId),
      getProfile(userId),
    ])
    return {
      username,
      name,
      groups,
      statusText: await getStatusText(username),
    }
  } catch (err) {
    if (err instanceof NotLoggedInError) {
      return null
    }
    throw err
  }
}



async function getGroups(userid) {
  await wait(50)
  if (userid !== 12) throw new NotLoggedInError('User not logged in')
  return ['group A', 'group B'];
}

async function getProfile(userid) {
  await wait(30)
  if (userid !== 12) throw new NotLoggedInError('User not logged in')
  return { username: 'grinchMaster135', name: 'The Grinch' };
}

async function getStatusText(username) {
  await wait(30)
  return 'away'
}

class NotLoggedInError extends Error {}

const wait = ms => new Promise(resolve => setTimeout(resolve, ms))


// TEST //

;(async function() {
  console.log('Grinch info:', await gatherUserInfo(12)) // Grinch info: { username: 'grinchMaster135', name: 'The Grinch', groups: [ 'group A', 'group B' ], statusText: 'away' }
  console.log('Non-logged in user info:', await gatherUserInfo(13)) // Non-logged in user info: null
})()

You could also have fixed the unhandled rejection by adding a no-op .catch handler to the Promise that was created before awaiting the other Promise:

groupsRequest.catch(() => void 0);

But that makes the code less elegant - I think the Promise.all version is better.

async function gatherUserInfo(userId) {
  try {
    // Don't await yet, so this request can be sent at the same time as future requests.
    const groupsRequest = getGroups(userId)
    groupsRequest.catch(() => void 0);

    const {username, name} = await getProfile(userId)
    return {
      username,
      name,
      groups: await groupsRequest,
      statusText: await getStatusText(username),
    }
  } catch (err) {
    if (err instanceof NotLoggedInError) {
      return null
    }
    throw err
  }
}



async function getGroups(userid) {
  await wait(50)
  if (userid !== 12) throw new NotLoggedInError('User not logged in')
  return ['group A', 'group B'];
}

async function getProfile(userid) {
  await wait(30)
  if (userid !== 12) throw new NotLoggedInError('User not logged in')
  return { username: 'grinchMaster135', name: 'The Grinch' };
}

async function getStatusText(username) {
  await wait(30)
  return 'away'
}

class NotLoggedInError extends Error {}

const wait = ms => new Promise(resolve => setTimeout(resolve, ms))


// TEST //

;(async function() {
  console.log('Grinch info:', await gatherUserInfo(12)) // Grinch info: { username: 'grinchMaster135', name: 'The Grinch', groups: [ 'group A', 'group B' ], statusText: 'away' }
  console.log('Non-logged in user info:', await gatherUserInfo(13)) // Non-logged in user info: null
})()

\$\endgroup\$
3
  • \$\begingroup\$ Both yours and @joseph's contributions were similar, but unique and helpful. Thanks! (if only I could accept both). I don't think adding the .catch() would work, because that would cause the error to be silently ignored, instead of being caught by the try-catch. Which, I guess shouldn't really be an issue because other endpoints would return the same logged-out error, and it'll get caught anyways. But yes, Promise.all would be better. \$\endgroup\$ Nov 2 '20 at 20:39
  • \$\begingroup\$ The no-op catch does work, you can run the second snippet to see - just because a Promise is being caught in one place doesn't stop it from being able to be caught in the other place. await somePromiseThatHasAlreadyRejected will properly reject the containing async function. \$\endgroup\$ Nov 2 '20 at 21:15
  • \$\begingroup\$ aah, I should have looked closer. I read it as groupRequest = getGroups(...).catch(...) but you were doing groupRequest = getGroups(...); groupRequest.catch(...), which are two very different things. \$\endgroup\$ Nov 2 '20 at 22:41

This site is temporarily in read only mode and not accepting new answers.

Not the answer you're looking for? Browse other questions tagged .