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

proof of concept for deterministic close race #101

Merged
merged 2 commits into from
Feb 14, 2019
Merged

Conversation

belm0
Copy link
Member

@belm0 belm0 commented Jan 27, 2019

No description provided.

@belm0 belm0 requested a review from mehaase January 27, 2019 12:45
@coveralls
Copy link

Pull Request Test Coverage Report for Build 21

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 92.048%

Totals Coverage Status
Change from base Build 19: 0.3%
Covered Lines: 382
Relevant Lines: 415

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jan 27, 2019

Pull Request Test Coverage Report for Build 31

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 92.754%

Totals Coverage Status
Change from base Build 27: 0.8%
Covered Lines: 384
Relevant Lines: 414

💛 - Coveralls

@@ -808,6 +811,8 @@ def __str__(self):

:param event:
'''
self._for_testing_peer_closed_connection.set()
await trio.sleep(0)
Copy link
Member Author

@belm0 belm0 Jan 27, 2019

Choose a reason for hiding this comment

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

the checkpoint could be eliminated in the common case at the cost of more complexity

def __init__():
   ...
   self._for_testing_peer_closed_connection = None

...

if self._for_testing_peer_closed_connection:
    self._for_testing_peer_closed_connection.set()
    await trio.sleep(0)

@mehaase
Copy link
Contributor

mehaase commented Feb 12, 2019

This is an interesting concept, but I wonder if this one execution path deserves special handling? There are other execution paths that race, too. I think I would rather rewrite some of the race-prone code now that I better understand some of the issues involving the reader task (e.g. #90), and then maybe have some kind of connection fuzzing script (a generalization of issue_96.py). Thoughts?

@belm0
Copy link
Member Author

belm0 commented Feb 14, 2019

Placing synchronization hooks at strategic points to induce known race scenarios has the advantage of supporting deterministic test cases. The fuzzing tests can get costly due to combinatorics and required resolution, and seem more appropriate as an offline tool to discover the races.

@belm0 belm0 merged commit 2453d81 into master Feb 14, 2019
@belm0 belm0 deleted the deterministic_close_race branch February 14, 2019 12:35
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.

3 participants