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

Catch KeyError as another possible failure of multithreading #710

Merged
merged 1 commit into from
Mar 6, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Mar 5, 2022

Description

When multithreading support is disabled, it is possible for operations on synced dicts to fail with a KeyError (as well as the other errors that were already being caught), so the multithreading test now catches that failure. Note that this is a bug in the test only, not in signac itself.

Motivation and Context

PyPy3 tests of multithreading have been failing due to this omission (see #705 and #706). Additionally, once this failure occurs, the buffer is in an invalid state, so future multithreading tests can fail in unpredictable ways. I reproduced the issue by running test_json_buffered_collection.py::TestBufferedJSONDictWriteConcern::test_multithreaded using pytest-repeat to run the test 100 times. I invariably observed failures within this number of tests. I then verified that catching the KeyError and clearing the collection prevents the error by running the test a few times with 1000 repeats (with pytest-xdist just to make sure that #705 was safe). I observed no further errors.

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality.

Checklist:

If necessary:

  • I have updated the API documentation as part of the package doc-strings.
  • I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
  • I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

@vyasr vyasr added the bug Something isn't working label Mar 5, 2022
@vyasr vyasr added this to the v1.8.0 milestone Mar 5, 2022
@vyasr vyasr requested review from a team as code owners March 5, 2022 18:53
@vyasr vyasr self-assigned this Mar 5, 2022
@vyasr vyasr requested review from b-butler and pepak13 and removed request for a team March 5, 2022 18:53
@vyasr vyasr mentioned this pull request Mar 5, 2022
12 tasks
@codecov
Copy link

codecov bot commented Mar 5, 2022

Codecov Report

Merging #710 (43e89eb) into master (2c6da9f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #710   +/-   ##
=======================================
  Coverage   78.54%   78.54%           
=======================================
  Files          65       65           
  Lines        7141     7141           
  Branches     1565     1565           
=======================================
  Hits         5609     5609           
  Misses       1228     1228           
  Partials      304      304           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c6da9f...43e89eb. Read the comment docs.

@bdice
Copy link
Member

bdice commented Mar 6, 2022

@vyasr What did the KeyError say in the message/error value?

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this is probably safe to merge if it fixed the issue locally for you. I am curious what the KeyError said.

@vyasr
Copy link
Contributor Author

vyasr commented Mar 6, 2022

@bdice have a look at the failure on #705, you can see the failure there. Here's a specific run.

@vyasr vyasr merged commit dfc28cb into master Mar 6, 2022
@vyasr vyasr deleted the fix/multithreaded_test branch March 6, 2022 01:29
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

Successfully merging this pull request may close these issues.

2 participants