bolt-js icon indicating copy to clipboard operation
bolt-js copied to clipboard

bug: bolt does not trigger handlers consistently when handler pattern contains stateful regex flags (`/y` or `/g`)

Open kefniark opened this issue 4 years ago • 4 comments

Description

Hi There,

Not really a Bolt-js issue, more an unexpected behavior and bad code from the net. You can close this issue when you want.

It took me quite some time to figure what was happening, so I'm posting it there if anyone run into the same kind of issue.

Problem:

  • calling a command /hello a first time works
  • calling the same command a second time ... timeout and /hello failed with the error "operation_timeout"
  • the server log only show one line, pointing to a ack() issue [ERROR] An incoming event was not acknowledged within 3 seconds. Ensure that the ack() argument is called in a listener.
  • after waiting a bit, I can run the command again, but only once
  • if I run another command, it works once, then timeout too
  • when it timeout, it doesnt even reach my listener or any part of my bolt code
  app.command(/(\/hello-dev|\/hello)/g, async ({ command, ack, respond }) => {
    await ack()
    await sayHello(command, respond)
  })

Solution: After debugging all over bolt and expressReceiver, I figured that the culprit is around here

The command regexp is tested with return pattern.test(candidate); and any flag immediately become problematic. My bad for copying a bad regexp from a bolt tutorial without noticing that.

So changing the regexp suddenly solved the problem (getting rid of flags like /g, /y)

app.command(/^\/(hello-dev|hello).*/, async ({ command, ack, respond }) => {
    await ack()
    await sayHello(command, respond)
})

Idea for improvement:

  • a warning message if not a single listener match, it would be way more helpful than a timeout
  • not showing a warning related to ack() if not a single listener match
  • a check on .lastIndex or .flags to check that regexp are not carrying a state over queries

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

  • [x] bug
  • [ ] enhancement (feature request)
  • [ ] question
  • [ ] documentation related
  • [ ] example code related
  • [ ] testing related
  • [x] 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.

kefniark avatar Aug 11 '21 10:08 kefniark

Hi @kefniark -- thanks for bringing this to our attention and going above and beyond by providing some ideas for improvement! 🙌🏼

This is really interesting (and unexpected behavior). I'm going to spend some time reproducing what you've reported and do some digging/debugging. I'll get back to you once I've done so!

misscoded avatar Aug 12 '21 19:08 misscoded

👋 It looks like this issue has been open for 30 days with no activity. We'll mark this as stale for now, and wait 10 days for an update or for further comment before closing this issue out.

github-actions[bot] avatar Nov 29 '21 20:11 github-actions[bot]

This issue is currently in our backlog and one that we still intend to investigate and introduce a fix, if appropriate.

misscoded avatar Dec 06 '21 22:12 misscoded