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

Timeout bugs in parallel variant #231

Open
benjaminpjones opened this issue Apr 6, 2024 · 7 comments
Open

Timeout bugs in parallel variant #231

benjaminpjones opened this issue Apr 6, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@benjaminpjones
Copy link
Collaborator

benjaminpjones commented Apr 6, 2024

Scenario: 4 players, two players time out in the same round -> round freezes

  • if "active" players try to play, they either change their move or get the "not enough time to change move" message
  • if timed-out players try to play, they get "not your turn"

Scenario: 4 players, one player times out -> game ends and says Player 0 wins!


I've looked at the code a bit, and this appears to be a bug in the way the variant handles "timeout", not the time system itself. So it's unrelated to recent changes in time control.

@merowin
Copy link
Collaborator

merowin commented Apr 6, 2024

About the Scenario with 4 players, one times out <- I've tested this in the past, and it used to work. I'm not sure what exactly is going wrong, it looks like all 4 players time out at (almost) the same time for some reason.

@merowin merowin added the bug Something isn't working label Apr 6, 2024
@merowin
Copy link
Collaborator

merowin commented Apr 6, 2024

I have found out the reason for this bug. The new function MakeTransition takes the move as input parameter. However in parallel time control, a move from any player may trigger a round transition. At this point we don't know the moves of other players, and it seems that the transition (with this move) is used for all players. In the event that the move is "timeout", the transition sets all player times to 0. Then all players immediately time out.

@merowin
Copy link
Collaborator

merowin commented Apr 6, 2024

I'm not sure how to fix it without disrupting your bigger PR.

@benjaminpjones
Copy link
Collaborator Author

Ohh oh no! Thank you for root causing. And sorry for breaking it 😬

I'm not sure how to fix it without disrupting your bigger PR.

Should I go ahead and land it then? Or I am also okay with dealing with some merge conflicts if we need to fix before landing

@merowin
Copy link
Collaborator

merowin commented Apr 6, 2024

Please go ahead and land the PR (when you are ready). All in all, I think It'll be easier to fix it afterwards.

And sorry for breaking it 😬

No problem! 😆

@benjaminpjones
Copy link
Collaborator Author

I believe the second issue "two players time out in the same round" is a race condition. It is very easy to trigger if it happens in the first timed round, but otherwise shouldn't happen often.

To repro:

  1. Set up 4 player parallel game (or any parallel variant with >2 players).
  2. Finish round 1 to start the clock
  3. play moves for player 0 and player 1
  4. Wait for player 2 and player 3 to timeout

Results:

It appears that the timeout for player 3 is overwritten or never occurs. If there is a race, it means that neither timeout will detect that the round ends.

Timeout in round two, simultaneous, BAD:

    "forPlayer": {
      "0": {
        "clockState": {
          "remainingTimeMS": 30000
        },
        "onThePlaySince": "2024-04-14T20:29:43.452Z",
        "stagedMoveAt": "2024-04-14T20:30:03.660Z"
      },
      "1": {
        "clockState": {
          "remainingTimeMS": 30000
        },
        "onThePlaySince": "2024-04-14T20:29:43.452Z",
        "stagedMoveAt": "2024-04-14T20:30:05.363Z"
      },
      "2": {
        "clockState": {
          "remainingTimeMS": 0
        },
        "onThePlaySince": null,
        "stagedMoveAt": null
      },
      "3": {
        "clockState": {
          "remainingTimeMS": 30000
        },
        "onThePlaySince": "2024-04-14T20:29:43.452Z",
        "stagedMoveAt": null
      }
    }

On the other hand, if the timeouts happen at different times, the round will get reset accordingly:

Here's a timeout in round 3, GOOD:

"forPlayer": {
      "0": {
        "clockState": {
          "remainingTimeMS": 26840
        },
        "onThePlaySince": "2024-04-14T20:38:37.323Z",
        "stagedMoveAt": null
      },
      "1": {
        "clockState": {
          "remainingTimeMS": 23038
        },
        "onThePlaySince": "2024-04-14T20:38:37.323Z",
        "stagedMoveAt": null
      },
      "2": {
        "clockState": {
          "remainingTimeMS": 0
        },
        "onThePlaySince": null,
        "stagedMoveAt": null
      },
      "3": {
        "clockState": {
          "remainingTimeMS": 0
        },
        "onThePlaySince": null,
        "stagedMoveAt": null
      }
    }

@benjaminpjones
Copy link
Collaborator Author

Linking related issue #226, which also needs a solution to race conditions

benjaminpjones added a commit that referenced this issue Apr 15, 2024
This rules out any issues with the variant itself. (debugging #231)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants