Skip to content

Conversation

@Debilski
Copy link
Member

@Debilski Debilski commented Sep 3, 2025

Fixes a bug (non-existent instance variable) introduced in #890 that caused longer timeouts and potential hangs during exit (including when __del__ is called on a game state).

@Debilski Debilski force-pushed the feature/fix-send_exit branch from 1fdc7a0 to 880aab5 Compare September 3, 2025 23:40
@Debilski
Copy link
Member Author

Debilski commented Sep 4, 2025

(Experimenting a little to find more race conditions; will take care of merging myself later)

@Debilski Debilski force-pushed the feature/fix-send_exit branch 2 times, most recently from 6c4d159 to be0c41f Compare September 6, 2025 22:36
@Debilski
Copy link
Member Author

Debilski commented Sep 7, 2025

This ended up becoming a tiny bit more involved. Previously, run_game would send an exit message (perhaps including a final game state – which is used by pelita server; or could be used by advanced bots for statistics) to the remote teams (exit_remote_teams) and then RemoteTeam.__del__ would be responsible for cleaning up any potential subprocesses.

The cleanup is now done a little more explicitly with an additional function cleanup_remote_teams. We now have a defined waiting period RemoteTeam.shutdown_timeout that we give a subprocess to shutdown cleanly before we terminate it. (A fallback is still present in RemoteTeam.__del__.)

In between I also experimented with having the remote team return a confirmation message after the exit request but it turned out to be a little useless for now.

Developer notes: The tricky part with debugging code like this is to always have an eye on the runtime of pytest. We typically want the tests to run as speedy as possible but there are some tests in which we actually expect a timeout to trigger (without this being explicitly mentioned in the test).

@Debilski Debilski force-pushed the feature/fix-send_exit branch from 7ad99bb to 1483a91 Compare September 7, 2025 09:49
@Debilski
Copy link
Member Author

Debilski commented Sep 8, 2025

Merging. Still not optimal (we could take more advantage of the state machine in the network code) but I’m somewhat confident that this leads to fewer random CI failures.

@Debilski Debilski merged commit 0ef3aac into ASPP:main Sep 8, 2025
29 of 30 checks passed
@Debilski Debilski deleted the feature/fix-send_exit branch September 8, 2025 09:34
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.

1 participant