WillFlame14 / hanabi-bot

A bot that plays on the hanab.live interface.
GNU General Public License v3.0
14 stars 9 forks source link

Crash RangeError: Maximum call stack size exceeded #323

Closed flackr closed 2 weeks ago

flackr commented 2 weeks ago

Version (PM the bot with /version): 1.4.13a Convention settings: /setall 5 Steps to reproduce or replay link: Unfortunately I lost the id for this since the link durning the game seems to be different than after it. I'll definitely add a replay if it happens again. Feel free to close if you think this is obsolete.

Additional information:

------- FINDING CLUES ------- 
file:///home/flackr/projects/hanabi-bot/src/tools/util.js:122
                return /** @type {T} */ (obj.map(elem => objClone(elem)));
                                                 ^

RangeError: Maximum call stack size exceeded
    at file:///home/flackr/projects/hanabi-bot/src/tools/util.js:122:36
    at [Array.map](http://array.map/) (<anonymous>)
    at objClone (file:///home/flackr/projects/hanabi-bot/src/tools/util.js:122:32)
    at objClone (file:///home/flackr/projects/hanabi-bot/src/tools/util.js:126:19)
    at file:///home/flackr/projects/hanabi-bot/src/tools/util.js:122:44
    at [Array.map](http://array.map/) (<anonymous>)
    at objClone (file:///home/flackr/projects/hanabi-bot/src/tools/util.js:122:32)
    at objClone (file:///home/flackr/projects/hanabi-bot/src/tools/util.js:126:19)
    at file:///home/flackr/projects/hanabi-bot/src/tools/util.js:122:44
    at [Array.map](http://array.map/) (<anonymous>)

Node.js v20.14.0

It looks like there may be an infinite recursion going on.

flackr commented 2 weeks ago

Possibly this could be related to setting the last action in evaluate clue: https://github.com/WillFlame14/hanabi-bot/blob/master/src/conventions/h-group/clue-finder/determine-clue.js#L38

As we discovered when landing this, the action includes a result which itself includes the game state IIRC. Perhaps we should shallow clone the action without the result.

WillFlame14 commented 2 weeks ago

I'm not sure if this is the same issue, but this replay also crashed with a maximum call stack size exceeded error. The stack trace appears to be in a different location, but this PR fixes the bug. Let me know if you have any thoughts.

Unfortunately I lost the id for this since the link durning the game seems to be different than after it.

The URL for a game while it is in progress includes the table id, like hanab.live/game/<table_id>. After the game ends (or is terminated), the URL changes to a shared replay and uses the replay id instead, like hanab.live/shared-replay/<replay_id>. You just need to make sure you copy the replay link and not the table link.

flackr commented 2 weeks ago

I'm not sure if this is the same issue, but this replay also crashed with a maximum call stack size exceeded error. The stack trace appears to be in a different location, but this PR fixes the bug. Let me know if you have any thoughts.

Right, I think the issue is this Object.assign on the clue: https://github.com/WillFlame14/hanabi-bot/blob/09fd1243ace0c8740bdad95881b44e0e4f28bd2d/src/conventions/h-group/clue-finder/clue-finder.js#L155

Combined with the clue being put in the last action results in trying to clone a recursive structure. This is why the PR would fix it - doing a clone of the clue means that it doesn't pick up the assigned resulting game.

WillFlame14 commented 2 weeks ago

Hmm, a ClueResult doesn't include a copy of the game though. In types.js, a ClueResult is defined as:

 * @typedef ClueResult
 * @property {number} elim
 * @property {Card[]} new_touched
 * @property {number} bad_touch
 * @property {number} trash
 * @property {number} avoidable_dupe
 * @property {number} remainder
 * @property {{playerIndex: number, card: Card}[]} playables
 * @property {{playerIndex: number, card: Card}[]} finesses
 * @property {ActualCard[]} chop_moved

so I'm not sure why the PR fixes it.

However, when looking for clues, I add the hypo_game to each SaveClue to help evaluate its value (around line 198 in clue-finder.js). Removing the hypo_game from the SaveClue after it's no longer needed also prevents the recursion error from occurring, but I still don't understand why cloning the clue fixed it. :shrug:

I suspect these are two unrelated bugs, since mine occurs in Utils.objEquals() and yours occurs in Utils.objClone().

flackr commented 2 weeks ago

I suspect these are two unrelated bugs, since mine occurs in Utils.objEquals() and yours occurs in Utils.objClone().

If some part of hypo_game.last_actions[giver] = hypo_game, then any attempt to perform a recursive operation over the whole of hypo_game (which both objEquals and objClone do) will recurse forever. I guess I was wrong about the recursive object being result, but I knew there was a reference to the game somewhere since we also had to change that test code to allow for the recursive reference.

I do absolutely think this is the same issue, and that my fix, or wholly cloning the action object, or doing some minimal copy of just the fields we need for last_actions is the right thing to do.

flackr commented 2 weeks ago

If it helps, here's a simple code snippet (similar to what's happening in the real case) you can try which shows the infinite recursion:

const game = {
  clue: {game: undefined}
}
game.clue.game = game;
Utils.objEquals(game, game);
WillFlame14 commented 2 weeks ago

I understand that there is a circular reference, but I'm not sure why this PR solves the issue. Maybe I'm missing something, but I don't think doing a shallow clone with {...} resolves circular reference issues.

If it helps, here's a simple code snippet (similar to what's happening in the real case) you can try which shows the infinite recursion:

const game = {
  clue: {game: undefined}
}
game.clue.game = game;
Utils.objEquals(game, game);

In this example, cloning the clue by adding game = {...game, clue: { ...game.clue }}; doesn't fix the issue. The circular reference is still there, and I get a stack overflow when I try to compare the objects.

flackr commented 2 weeks ago

Ah sorry, I wasn't quite sure which part you were confused about.

We copy the clue before the circular reference is added by determine_clue.

The clue with the circular reference attached is as far as I can tell, never added to last_action because by the time it's added there it's not the same clue object.

WillFlame14 commented 2 weeks ago

Makes sense, thanks!