online-go / gtp2ogs

GTP Wrapper to allow bots to interface with the Online-Go.com Server
MIT License
88 stars 29 forks source link

Fix ending_bot crash #411

Closed raylu closed 1 year ago

raylu commented 1 year ago

when ending_bot is undefined, this.ending_bot?.sendMove(...) evaluates to undefined. this means we are calling ignore_promise(undefined), which is how we get

Aug 15 09:25:08 ! source-map-support.js:445 /snapshot/bench/gtp2ogs/dist/webpack:/gtp2ogs/src/util.ts:110
    promise.catch((e) => {
            ^
Aug 15 09:25:08 ! source-map-support.js:448 TypeError: Cannot read properties of undefined (reading 'catch')
    at ignore_promise (/snapshot/bench/gtp2ogs/dist/webpack:/gtp2ogs/src/util.ts:110:13)
    at GobanSocket.on_move (/snapshot/bench/gtp2ogs/dist/webpack:/gtp2ogs/src/Game.ts:316:39)
    at GobanSocket.emit (/snapshot/bench/gtp2ogs/node_modules/eventemitter3/index.js:181:35)

closes #410

Dorus commented 1 year ago
  1. i'm unsure why the this.ending_bot? did not work, what version of node did you run?
  2. Why the doc changes in a crash fix?
  3. Why is ending bot always black?
raylu commented 1 year ago
  1. even with the ?, this.ending_bot?.sendMove(...) still evaluates to undefined, so we're still calling ignore_promise(undefined), which then tries to do undefined.catch(...)
  2. would you prefer it in a separate PR?
  3. in the handicap case, we check that we're not black above, so we must be white, so our opponent (who we are recording moves for in the bot) is always black when we reach that code
Dorus commented 1 year ago
  1. ah that's valid. Could also be solved by fixing ignore_promise to use promise?.catche at util.ts:110 but your solution is valid.
  2. not up to me, if both changes are accepted that's fine, but yes i would prefer separate pr's for separate changes.
  3. also valid, i didn't check the surrounding code and was just curious.
github-actions[bot] commented 1 year ago

This pull request has been marked stale and will be closed soon without further activity. Please update the PR or respond to this comment if you're still interested in working on this.

raylu commented 1 year ago

would still like to get this merged

Dorus commented 1 year ago

Are we sure the current patch work?

I was thinking we need a question mark in ignore_promise (utils.ts)

export function ignore_promise(promise: Promise<any>): void {
    promise?.catch((e) => {
        trace.error(e);
    });
}

line 2 promise?.catch

Edit: nevermind, i again missed that if statement that also fixes it. This patch is fine.