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 minheap as timeout container and fix collision of multiple SelectConnection timeouts with same deadline and callback #947

Merged
merged 26 commits into from Feb 16, 2018

Conversation

Projects
None yet
2 participants
@vitaly-krugl
Member

vitaly-krugl commented Feb 5, 2018

(Note that the failed coverage test results from pika's encrypted credentials configuration issue.)

  1. NOTE: On Windows, hash(frozenset(value.items())) used by the legacy implementation of SelectConnection's _PollerBase.add_timeout readily results in timer id collision if multiple timers have very close deadlines and same callback. See failed TestViabilityOfMultipleTimeoutsWithSameDeadlineAndCallback test results in https://ci.appveyor.com/project/gmr/pika/build/1.0.409. This is aided by Windows time.time() having lower resolution than on Linux. However, on a faster machine, the same problem would also likely show up on linux, and hashes aren't guaranteed to be unique for all input set. And I recall getting it on my dev MacBookPro a long time ago.
  2. A minheap is a more natural and efficient container for managing timeouts.
Vitaly Kruglikov Vitaly Kruglikov

@vitaly-krugl vitaly-krugl changed the title from DO NOT MERGE Implemented BlockingConnection test TestViabilityOfMultipleTimeoutsWithSameDeadlineAndCallback to DO NOT MERGE Fix collision of multiple SelectConnection timeouts with same deadline and callback Feb 5, 2018

Vitaly Kruglikov Vitaly Kruglikov

@vitaly-krugl vitaly-krugl force-pushed the vitaly-krugl:fix-multiple-timers-same-deadline branch 2 times, most recently from 970c2d3 to 5bd1a04 Feb 5, 2018

@vitaly-krugl vitaly-krugl force-pushed the vitaly-krugl:fix-multiple-timers-same-deadline branch from 5495577 to 676e7e9 Feb 5, 2018

@vitaly-krugl vitaly-krugl force-pushed the vitaly-krugl:fix-multiple-timers-same-deadline branch from 8b7f4ff to 84b1061 Feb 7, 2018

@vitaly-krugl vitaly-krugl force-pushed the vitaly-krugl:fix-multiple-timers-same-deadline branch from 84b1061 to 90a32c8 Feb 7, 2018

@vitaly-krugl vitaly-krugl changed the title from DO NOT MERGE Fix collision of multiple SelectConnection timeouts with same deadline and callback to DO NOT MERGE Use heap as timeout container and fix collision of multiple SelectConnection timeouts with same deadline and callback Feb 7, 2018

@vitaly-krugl vitaly-krugl changed the title from DO NOT MERGE Use heap as timeout container and fix collision of multiple SelectConnection timeouts with same deadline and callback to DO NOT MERGE Use minheap as timeout container and fix collision of multiple SelectConnection timeouts with same deadline and callback Feb 7, 2018

@vitaly-krugl vitaly-krugl changed the title from DO NOT MERGE Use minheap as timeout container and fix collision of multiple SelectConnection timeouts with same deadline and callback to Use minheap as timeout container and fix collision of multiple SelectConnection timeouts with same deadline and callback Feb 10, 2018

@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Feb 10, 2018

cc @lukebakken

Ready for review

vitaly-krugl added some commits Feb 10, 2018

Use weak references to save callbacks passed by select_connection's I…
…OLoop to the poller in order to prevent cyclical references that interfere with garbage collection.
@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Feb 12, 2018

I tried to use weakref and it resulted in test failures. I will be working through it shortly

@vitaly-krugl vitaly-krugl requested a review from lukebakken Feb 12, 2018

@lukebakken lukebakken added this to the 1.0.0 milestone Feb 12, 2018

Don't use weakref.proxy to store references to callback instance meth…
…ods, since instance methods have temporary references
@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Feb 12, 2018

@lukebakken - this is ready for review. Thanks!

@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Feb 15, 2018

@lukebakken, do you anticipate an opportunity to review this in the near future? My next PR #956 that implements the thread-safe callback request feature in BlockingConnection and SelectConnection builds on this PR. Thx!

@lukebakken

This comment has been minimized.

Contributor

lukebakken commented Feb 15, 2018

@vitaly-krugl I'll get it today. I didn't notice that "DO NOT MERGE" had been removed from the title.

@lukebakken

This looks great and tests pass locally and on Travis

@vitaly-krugl vitaly-krugl merged commit 0e13941 into pika:master Feb 16, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@vitaly-krugl

This comment has been minimized.

Member

vitaly-krugl commented Feb 16, 2018

Thank you for reviewing, @lukebakken!

lukebakken added a commit that referenced this pull request Apr 16, 2018

Merge pull request #947 from vitaly-krugl/fix-multiple-timers-same-de…
…adline

Use minheap as timeout container and fix collision of multiple SelectConnection timeouts with same deadline and callback

(cherry picked from commit 0e13941)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment