Calls to async.applyEachSeries must include an explicit callback

[UppaJung]

When you call async.applyEachSeries with a sequence of arguments that ends in a function, it assumes that the final argument is a callback that should be triggered when the series is complete.

https://caolan.github.io/async/docs.html#applyEachSeries

In the use cases fixed in this PR, async.applyEachSeries is used to call middleware where the final parameter to the middleware is the next function.

Since the next is a function, and was the final parameter to applyEachSeries, applyEachSeries assumed it was the optional callback that can be provided as a final parameter, and not the final parameter to be passed to the middleware. When calling the middleware, undefined was sent in place of the next parameter. Middleware that actually needed to call the next parameter received an undefined value.

This PR ensures that the next function is indeed passed as the final parameter to the middleware.

[BenV]

Not sure if this is a related issue or not, but I get the same issue with next being undefined if I use an async function as a callback:

server.addMiddleware(server.MIDDLEWARE_PUBLISH_IN, async(req, next) => next());

This worked fine with SC 9.x, the issue only surfaced once we upgraded to a more recent version.

[jondubois]

@UppaJung Sorry for the delay in response. The current code is intended behaviour, the last function passed to the applyEachSeries call is not meant to be the next callback; it's the final function that should be run after all the middleware functions have run (note that it's possible to add multiple middleware functions for each type of middleware and they are chained one after the other).

The next callbacks passed to individual middleware function (which you invoke in your code) is actually provided by the module, async will call the final callback when all functions in the chain have called next OR one of those functions calls next with an error next(err).

[jondubois]

@BenV Support for this use case was not added explicitly but I'll try to fix this.

[jondubois]

@UppaJung Could you confirm if your middleware function had the current number of arguments? Could this be the root of your issue or something else? The test cases which cover middleware are passing. Note that middleware functions should be in the form function (req, next) {} (first argument is req, second argument is next).

[jondubois]

@BenV I tested your use case, passing an async function (as in async-await) is supported but if you do this, then the async function must return a new Promise and then either resolve() or reject(err) the promise (instead of calling the callback next() or next(err) respectively). This change may have happened when SC upgraded the version of the async module (now it seems to take into account async functions as being special; whereas before it just treated it like any regular function).

[BenV]

Ahh interesting, makes sense. Thanks for looking into it!

[UppaJung]

@jondubois: If the SocketCluster code can be called in a way that causes undefined to be passed to a "next" function, does that not imply there's either a bug or the parameters aren't being checked correctly?

Here's the code we have been using, with both a error print out if next is undefined and some extra code to make sure that never happens even if SocketCluster passes us a null next function. It indeed has a function which is of the form (req, next).

    // add middleware for subscribing
    scServer.addMiddleware(
      scServer.MIDDLEWARE_SUBSCRIBE,
      async (
        req: { channel: string, socket: { getAuthToken: () => Types.UnencodedUserTokenObject } },
        // Hack until a bug is fixed in SocketCluster's server
        next: (error?: any) => void = (error?: any) => {
          if (typeof(error) !== "undefined") {
            // tslint:disable-next-line:no-console
            console.log("Error in addMiddleware", error);
            throw(error);
          }
        }
      ) => {
        let channelNameParameter: Types.Channels.NameParameters;
        // tslint:disable-next-line:no-console
        // console.log("typeof(next)", next, req);
        // parse channelName to channelNameParameter object
        try {
          channelNameParameter = JSON.parse(req.channel);
        } catch (err) {
          // throw error if channel is not stringified json object
          next(err);
          return;
        }

        // get user object from socket server
        const unencodedUserTokenObject = req.socket.getAuthToken();
        const {authenticatedUserId} = unencodedUserTokenObject;

        // authenticate whether current user can subscribe the channel or not
        let isAuthenticated: boolean;
        try {
          isAuthenticated = await authenticateSubscription(authenticatedUserId, channelNameParameter);
        } catch (err) {
          next(err);
        }

        if (isAuthenticated) {
          next();
        } else {
          next(`${authenticatedUserId || "anonymous user"} does not have access to subscribe to ${req.channel}${
            unencodedUserTokenObject ?
              "no unencodedUserTokenObject" :
              JSON.stringify({unencodedUserTokenObject})
          }`);
        }
      }
    );

[BenV]

@UppaJung It sure looks like you're running into the same issue I was with using async/await. I got around this by simply using a non-async wrapper function for the middleware logic, but it sounds like you can also return a promise instead of using next.

[UppaJung]

@jondubois: Is the behavior for async/await documented anywhere? I can change my code to work the way you say it's supposed to work, but there's at least a documentation bug if nobody who reads the documentation knows this is how it's supposed to work. When I look at the documentation for addMiddleware, there's nothing that tells me that if my function returns a promise I can't also call next.