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

Fix ending_bot crash #411

Merged
merged 1 commit into from
Oct 30, 2023
Merged

Fix ending_bot crash #411

merged 1 commit into from
Oct 30, 2023

Conversation

raylu
Copy link
Contributor

@raylu raylu commented Sep 24, 2023

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
Copy link
Contributor

Dorus commented Sep 27, 2023

  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
Copy link
Contributor Author

raylu commented Sep 27, 2023

  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
Copy link
Contributor

Dorus commented Sep 27, 2023

  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
Copy link

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.

@github-actions github-actions bot added the stale label Oct 29, 2023
@raylu
Copy link
Contributor Author

raylu commented Oct 29, 2023

would still like to get this merged

@Dorus
Copy link
Contributor

Dorus commented Oct 29, 2023

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.

@github-actions github-actions bot removed the stale label Oct 30, 2023
@roy7 roy7 merged commit 58310d2 into online-go:devel Oct 30, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crash in ignore_promise reading 'catch'
3 participants