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

Use threading.RLock() locks for global values in chatcommunicate.py #7033

Merged
merged 3 commits into from
May 28, 2022

Conversation

makyen
Copy link
Contributor

@makyen makyen commented May 27, 2022

In CI testing, we've been getting a notable number of errors in test/test_chatcommunicate.py and some other affected tests. These have indicated real thread safety issues in chatcommunicate.py (and some in the tests themselves). This PR is intended to resolve those issues.

In order to resolve the thread safety issues in chatcommunicate.py and resolve some issues in CI testing this PR:

  • Adds and uses threading.RLock() locks for each of the global values in chatcommunicate.py, or groups of values which are used in similar areas, within chatcommunicate.py.
  • Uses those locks in other modules where those modules directly access and/or manipulate the values protected by a lock.
  • Adds some decorators to some tests in test_chatcommands.py to store and restore some of the values which are being protected by the locks. Those decorators are used where the tests intentionally clobber the protected values.
  • Adjusts the code in test_validate_yaml() to perform some additional tests and display the results for all users, rather than failing after a single problem user and reporting only that one, when there might be additional issues.

These changes should largely resolve the failing tests noted in issue #6955. It's possible that there are remaining issues which are not addressed here, but this should be the bulk of the failing tests I've seen reported.

Testing of the changes in this PR was done in the Makyen Test 02 chat room around here for about 4 hours of running a test SD instance doing normal scanning and some chat commands. The latter two commits were not included in that testing, but those commits affect only CI testing.

@makyen
Copy link
Contributor Author

makyen commented May 28, 2022

We've had 3 more CI failures in the last 24 hours which should be resolved by the changes in this PR.

Adding/adjusting locks is always something which comes with some risk of introducing a deadlock. I would be more comfortable merging this on a weekend, when SE sites are less busy and uninterrupted SD operation is less critical. Thus, I'm going to go ahead and merge this, so it's first introduced over a weekend.

@makyen makyen merged commit 53c0163 into master May 28, 2022
@makyen makyen deleted the Mak-use-RLocks-for-global-values-in-chatcommunicate branch May 28, 2022 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

1 participant