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
Switch to deque LifoQueue #1356
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1356 +/- ##
======================================
Coverage 100% 100%
======================================
Files 21 22 +1
Lines 2021 2030 +9
======================================
+ Hits 2021 2030 +9
Continue to review full report at Codecov.
|
urllib3/connectionpool.py
Outdated
# Queue is imported for side effects on MS Windows | ||
import Queue as _unused_module_Queue # noqa: F401 | ||
from .util.queue import LifoQueue | ||
from .packages.six.moves import queue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this move down here? The rest of the imports from packages
live elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason, I deleted it and upon realizing that queue.Empty
was used I put it back but not in the right place. I'll fix it and amend the commit.
urllib3/util/queue.py
Outdated
from ..packages.six.moves import queue | ||
|
||
if six.PY2: | ||
# Queue is imported for side effects on MS Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No kidding? Got a link that explains what these side-effects are and why they're useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me find the commit where that was added in the blame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready for re-review. :) |
Might need an admin force merge for this one until AppVeyor is back :) |
Hrm, is appveyor still not reporting? I'll need to look into that a bit more closely on Friday. |
Switch to using our own LifoQueue that uses a
collections.deque
backend which is a lot faster than a list backend. Closes #1087.