Node-slack-sdk: [socket-mode] webClient error is swallowed

[g12i]

Description

When webClient.apiCall throws an error it is not propagated and handled by onFailure. Instead an error is logged and forgotten.

              return this.webClient.apiCall('apps.connections.open').then((result: WebAPICallResult) => {
                return result;
              }).catch((error) => {
                this.logger.error(error);
              });

As a result machine changes state to authenticated handled where context.result is undefined

Which ultimately leads to TypeError: Cannot read property 'url' of undefined in

                this.setupWebSocket(context.result.url);

What type of issue is this? (place an x in one of the [ ])

• [x] bug
• [ ] enhancement (feature request)
• [ ] question
• [ ] documentation related
• [ ] testing related
• [ ] discussion

Requirements (place an x in each of the [ ])

• [x] I've read and understood the Contributing guidelines and have done my best effort to follow them.
• [x] I've read and agree to the Code of Conduct.
• [x] I've searched for any related issues and avoided creating a duplicate issue.

---

Bug Report

Filling out the following details about bugs will help us solve your issue sooner.

Packages:

Select all that apply:

• [ ] @slack/web-api
• [ ] @slack/events-api
• [ ] @slack/interactive-messages
• [ ] @slack/rtm-api
• [ ] @slack/webhooks
• [ ] @slack/oauth
• [ ] I don't know
• [x] @slack/socket-mode

Reproducible in:

package version: 1.0.0

node version: v12.18.4

OS version(s): macOS Catalina

Steps to reproduce:

1. Provide wrong app token.
2. Start bolt with socketMode: true

Expected result:

Error is rethrown after logging and state machine does not transition to "authenticated".

Actual result:

Error is swallowed and state machine transitions to "authenticated".

Attachments:

⚡️ Bolt app started
[DEBUG]  web-api:WebClient:0 http response received
[ERROR]  socket-mode:SocketModeClient:0 Error: An API error occurred: not_authed
    at Object.platformErrorFromResult (/Users/#####/node_modules/@slack/socket-mode/node_modules/@slack/web-api/dist/errors.js:51:33)
    at WebClient.apiCall (/Users/#####/node_modules/@slack/socket-mode/node_modules/@slack/web-api/dist/WebClient.js:156:28)
    at processTicksAndRejections (internal/process/task_queues.js:97:5) {
  code: 'slack_webapi_platform_error',
  data: {
    ok: false,
    error: 'not_authed',
    response_metadata: { acceptedScopes: [Array] }
  }
}
[DEBUG]  socket-mode:SocketModeClient:0 transitioning to state: connecting:authenticated
(node:92065) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'url' of undefined
    at /Users/#####/node_modules/@slack/socket-mode/dist/SocketModeClient.js:76:48
    at /Users/#####/node_modules/finity/lib/utils/invokeEach.js:11:15
    at Array.forEach (<anonymous>)
    at invokeEach (/Users/#####/node_modules/finity/lib/utils/invokeEach.js:10:7)
    at StateMachine.enterState (/Users/#####/node_modules/finity/lib/core/StateMachine.js:125:32)
    at StateMachine.executeTransition (/Users/#####/node_modules/finity/lib/core/StateMachine.js:116:12)
    at /Users/#####/node_modules/finity/lib/core/StateMachine.js:285:16
    at TaskScheduler.execute (/Users/#####/node_modules/finity/lib/core/TaskScheduler.js:29:7)
    at StateMachine.executeTrigger (/Users/#####/node_modules/finity/lib/core/StateMachine.js:282:24)
    at StateMachine.handleAsyncActionComplete (/Users/#####/node_modules/finity/lib/core/StateMachine.js:195:10)
(node:92065) 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: 1)
(node:92065) [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.
[DEBUG]  web-api:WebClient:0 http response received

[mwbrooks]

Hey @g12i, thanks a bunch for submitting this issue and providing the details to reproduce it.

I've reproduced it on my end and we definitely agree that this is a bug. We'll put together a pull request to properly fail the state machine rather than allowing it to become authenticated.

[stevengill]

So after throwing an error here (which I agree we should definitely be doing), the state will correctly go to failed and then disconnected. This means, for node.js not to complain about an unhandled promise rejection, you will need to add a try/catch around your app.start.

Example:

(async () => {
  try {
    await app.start();
    console.log('⚡️ Bolt app started');
  } catch (err) {
    console.log('err with start', err)
  }
})();

I was wondering what others thought about this. Is this a good design?

Alternatively, I could not propagate the error to start and instead send it to the global error handler app.error, but this isn't a error that can be recovered from so not sure that makes sense. Would the process keep running in this instance even though the websocket is disconnected? Doesn't make sense.

Thoughts @seratch @aoberoi?

[seratch]

@stevengill
I am not sure the following code is the best solution yet but I am thinking that we may want to have an error handling for TypeError: Cannot read property 'url' of undefined in this scenario. As the apps.connections.open API method call returns 200 OK even for an error response, the state machine goes to .onSuccess().transitionTo('authenticated') pattern. In this instance, the context.result can be unexpectedly undefined.

https://github.com/slackapi/node-slack-sdk/blob/%40slack/socket-mode%401.0.0/packages/socket-mode/src/SocketModeClient.ts#L99-L107

.state('authenticated')
.onEnter((_state, context) => {
  this.authenticated = true;
  if (typeof context.result == "undefined") {
    // ok: false
    setImmediate(() => {
      this.emit('failed', context.result);
    });
  } else {
    this.setupWebSocket(context.result.url);
    setImmediate(() => {
      this.emit('authenticated', context.result);
    });
  }
})
.on('websocket open').transitionTo('handshaking')

With this change, (async () => { await app.start(); })(); simply exists exits this way:

$ npx node index.js
⚡️ Bolt app started
[ERROR]  socket-mode:SocketModeClient:0 Error: An API error occurred: invalid_auth
    at Object.platformErrorFromResult (/Users/ksera/github/bolt-starter/node_modules/@slack/socket-mode/node_modules/@slack/web-api/dist/errors.js:51:33)
    at WebClient.apiCall (/Users/ksera/github/bolt-starter/node_modules/@slack/socket-mode/node_modules/@slack/web-api/dist/WebClient.js:156:28)
    at processTicksAndRejections (internal/process/task_queues.js:97:5) {
  code: 'slack_webapi_platform_error',
  data: { ok: false, error: 'invalid_auth', response_metadata: {} }

[g12i]

If I may enter the discussion I'd say @stevengill 's approach seems to be going by the book.

Since error handling already exist I'd say it should be used. That also gives the application an opportunity to recover, if the user sets up retry strategy (if I understand this correctly). This if in "authenticated" state creates a precedence, increases the complexity and introduces something that could be easily missed during further development.

That being said, users must be aware that "start()" may produce an error. It sounds obvious now that I know it, but somehow I thought it would be propagated to global error handler. Which doesn't make sense - it's like expecting express's listen method to propagate an error to error handling middleware. A note in Bolt documentation shoud do the trick.

[stevengill]

Yup, I'm definitely leaning towards the error propagating to app.start().

@seratch one issue with the existing code, is that the following code does in fact throw an error that is caught by catch

  return this.webClient.apiCall('apps.connections.open').then((result: WebAPICallResult) => {
    return result;
  }).catch((error) => {
    this.logger.error(error);
  });

It seems like this might be a perfect usecase to use the failure callback. Letting it go on to the success callback seems incorrect (and puts the state machine into an incorrect state). I could direct the state machine in the success callback back to disconnected state when handling the url error. With your solution @seratch, the bolt process doesn't end. Seems weird to me to keep the process running even if the socket never connected.

This error seems kind of like the port in use error which also throws an error on app.start() and gives the UnhandledPromiserejectionWarning when not handled with a try/catch

[seratch]

@stevengill

one issue with the existing code

Good point - I somehow overlooked the part.

With your solution @seratch, the bolt process doesn't end. Seems weird to me to keep the process running even if the socket never connected.

I've checked my code again. I found that the process ends but it does not exit with non-zero code. You can verify the behavior by the steps - 1) create your sample Socket Mode app with an invalid appToken 2) run npx node index.js && echo $?. Anyway, I do not think we should go with this solution at all.

In my understanding, the goal in this issue is to enable developers to write error handlers like the following one (correct me if I am wrong).

(async () => {
  try {
    await app.start();
    console.log('⚡️ Bolt app started');
  } catch (err) {
    console.log('Failed to start Bolt app', err);
    process.exit(1);
  }
})();

As @g12i mentioned, the code is more idiomatic. More importantly, enabling this should be useful for better production operations such as detecting invalid deployments and/or invalidation of tokens. One minor concern I have is that it looks a bit long ... but it should be acceptable for production apps.

[stevengill]

Sent some PRs fixing the issue

[stevengill]

Pull requests merged! Will have to do a release today/tomorrow with the fixes