Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PuppetWeb loses event listeners when resetting #1470

Closed
hkusb opened this issue Jul 12, 2018 · 6 comments
Closed

PuppetWeb loses event listeners when resetting #1470

hkusb opened this issue Jul 12, 2018 · 6 comments
Labels

Comments

@hkusb
Copy link

hkusb commented Jul 12, 2018

Why setImmediate needed when removing listeners of the bridge in PuppetWeb.quit(). It seems a bug when resetting puppet. It causes the bridge to lose some event listeners like logout.
Wechaty version: 0.14.4

public initWatchdogForPuppet(): void {
    log.verbose('PuppetWeb', 'initWatchdogForPuppet()')

    const puppet = this
    const dog    = this.puppetWatchdog

    // clean the dog because this could be re-inited
    dog.removeAllListeners()

    puppet.on('watchdog', food => dog.feed(food))
    dog.on('feed', food => {
      log.silly('PuppetWeb', 'initWatchdogForPuppet() dog.on(feed, food={type=%s, data=%s})', food.type, food.data)
      // feed the dog, heartbeat the puppet.
      puppet.emit('heartbeat', food.data)
    })

    dog.on('reset', async (food, timeout) => {
      log.warn('PuppetWeb', 'initWatchdogForPuppet() dog.on(reset) last food:%s, timeout:%s',
                            food.data, timeout)
      try {
        await this.quit()
        await this.init()
      } catch (e) {
        puppet.emit('error', e)
      }
    })
  }
public async quit(): Promise<void> {
    log.verbose('PuppetWeb', 'quit()')

    const off = this.state.off()
    if (off === 'pending') {
        const e = new Error('quit() is called on a PENDING OFF PuppetWeb')
        log.warn('PuppetWeb', e.message)
        this.emit('error', e)
        return
    } else if (off === true) {
        log.warn('PuppetWeb', 'quit() is called on a OFF puppet. return directly.')
        return
    }

    log.verbose('PuppetWeb', 'quit() make watchdog sleep before do quit')
    this.puppetWatchdog.sleep()
    this.scanWatchdog.sleep()

    this.state.off('pending')

    try {
      await this.bridge.quit()
      // register the removeListeners micro task at then end of the task queue
      setImmediate(() => this.bridge.removeAllListeners())
    } catch (e) {
      log.error('PuppetWeb', 'quit() exception: %s', e.message)
      Raven.captureException(e)
      throw e
    } finally {
      this.state.off(true)
    }
  }
public async initBridge(profile: Profile): Promise<Bridge> {
    log.verbose('PuppetWeb', 'initBridge()')

    if (this.state.off()) {
      const e = new Error('initBridge() found targetState != live, no init anymore')
      log.warn('PuppetWeb', e.message)
      throw e
    }

    const head = config.head
    // we have to set this.bridge right now,
    // because the Event.onXXX might arrive while we are initializing.
    this.bridge = new Bridge({
      head,
      profile,
    })

    this.bridge.on('ding'     , Event.onDing.bind(this))
    this.bridge.on('error'    , e => this.emit('error', e))
    this.bridge.on('log'      , Event.onLog.bind(this))
    this.bridge.on('login'    , Event.onLogin.bind(this))
    this.bridge.on('logout'   , Event.onLogout.bind(this))
    this.bridge.on('message'  , Event.onMessage.bind(this))
    this.bridge.on('scan'     , Event.onScan.bind(this))
    this.bridge.on('unload'   , Event.onUnload.bind(this))

    try {
      await this.bridge.init()
    } catch (e) {
      log.error('PuppetWeb', 'initBridge() exception: %s', e.message)
      await this.bridge.quit().catch(console.error)
      this.emit('error', e)

      Raven.captureException(e)
      throw e
    }

    return this.bridge
  }

When puppet's watchdog receives reset, pupput.quit() and puppet.init() will be called. But there is setImmediate(() => this.bridge.removeAllListeners()) in quit(). It means this.bridge.removeAllListeners() will be called after puppet.init() called. That is, it will remove the new bridge's listeners.

Steps to reproduce

  1. Start a wechaty bot.
  2. Kill chrome process.
    PuppetWeb will receive reset and chrome will recover after one minute. And then you won't receive logout and message any more.
@hkusb
Copy link
Author

hkusb commented Jul 12, 2018

It works well after I remove setImmediate

@huan
Copy link
Member

huan commented Jul 12, 2018

Hello @hkusb!

Thanks for reporting this.

I had just checked the source code and I believe this bug had been removed for the latest version of Wechaty/PuppetPuppeteer.

Could you please try with the Wechaty v0.18 or above version?

Please let me know the result, and if the problem still exists, please file an issue at the PuppetPuppeteer Repository at https://github.com/Chatie/wechaty-puppet-puppeteer because it had been moved to its new home!

BTW: I should warn you before you get started is that there has many BREAKING CHANGES between the v0.14 and v0.16. You can get more information from the release notes at https://github.com/Chatie/wechaty/releases/tag/v0.16.0 and our blog at https://blog.chatie.io/migrating-wechaty-v0.14-to-v0.18-guide-from-puppeteer-to-padchat-en/

huan added a commit that referenced this issue Jul 12, 2018
@huan
Copy link
Member

huan commented Jul 12, 2018

Update:

For your convenience, I had removed the setImmediate in v0.14 branch, it should have been fixed with Wechaty v0.14.12 or above versions.

Please feel free to let me know if there have any other problems with it.

@huan huan added the bug label Jul 12, 2018
@huan
Copy link
Member

huan commented Jul 19, 2018

@hkusb Could you please confirm that whether this issue is resolved by my modification?

@hkusb
Copy link
Author

hkusb commented Jul 20, 2018

Sorry, I've just seen it. It works very well for me. And I'm sorry for that I have no time to try it on v0.18 recently. I'll try it later.

@hkusb hkusb changed the title PupppetWeb loses event listeners when resetting PuppetWeb loses event listeners when resetting Jul 20, 2018
@huan
Copy link
Member

huan commented Jul 20, 2018

Thank you for the confirmation.

I'll close this issue and please file a new issue if there's any problem with the version v0.18 or above.

@huan huan closed this as completed Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants