Create custom Selectors (Kqueue, Epoll, Poll, and Select) Backport #1001

sethmlarson · 2016-10-19T03:01:50Z

This PR adds all the select methods to a new util module called wait. Uses the same ordering used by selectors.py in the standard library to determine which selector should be used. Also factored in a way to allow easy patching and testing. Similar to #998 but uses a different way to break up the different methods to wait for I/O events. Also adds tests to exercise the selectors which can probably be extended.

Lukasa · 2016-10-19T07:23:47Z

@SethMichaelLarson Rather than us doing this, would it be better simply to vendor Python 3.5's selectors module? Or, possibly, just have selectors as an optional dependency for using something other than select? I'm a bit inclined to want to take a general solution that someone else built rather than write our own custom code that is more likely to be partial or incomplete.

Thoughts?

sethmlarson · 2016-10-19T12:17:09Z

@Lukasa This is what asyncio does, while possible I think it may be overkill in this situation as a selector has much more overhead compared to a polling object. Asyncio has a different use-case for those selectors to what we're using them for.

Lukasa · 2016-10-19T12:17:53Z

@SethMichaelLarson Do we have a good feel on what the overhead is?

sethmlarson · 2016-10-19T12:28:01Z

@Lukasa Probably minimal but I can take a look if you're interested. I'm +1 on the vendoring selectors suggestion. In the future if we get to doing your suggestion of having IOCP being used for selecting on Windows we'll have to have our own separate module as well but for now this is the best solution.

Lukasa · 2016-10-19T12:28:45Z

Heh, if we get to the point of doing IOCP we're going to be in a whole separate world of pain anyway. ;)

sethmlarson · 2016-10-19T12:29:21Z

So I'll open a separate PR or just keep it in this one?

Lukasa · 2016-10-19T12:29:58Z

Let's keep the work here and just retitle the PR a bit.

sethmlarson · 2016-10-19T12:30:26Z

Sure thing, will re-title when I commit.

sethmlarson · 2016-10-19T14:04:37Z

Lukasa · 2016-10-19T14:08:30Z

Nope, looks good. By the time we get to the last bullet we should come to a position on whether we want to use something like coveralls for test coverage across multiple platforms, but let's do one step at a time.

sethmlarson · 2016-10-19T14:10:08Z

I'd recommend codecov over coveralls, just better functionality and integration with Github. /bikeshed 😉 But honestly, being able for it to comment on PRs is nice.

Lukasa · 2016-10-19T14:10:48Z

Let's not put the cart before the horse. ;) Let's get kqueue support first, and then worry about testing it. =D

sethmlarson · 2016-10-19T14:12:34Z

@Lukasa Agreed!

sethmlarson · 2016-10-19T16:19:12Z

So I've done a little work and implemented SelectSelector to start us off as it's the simplest. The biggest part of commit 983a193 is the _syscall_wrapper function. This function is made to wrap system calls and to retry them if they fail in the middle of executing due to an EINTR error. Usage of this function is required within all system calls for this module as Python 3.4< does not handle this themselves, instead returning an empty list.

I've decided to keep the wait_for_read and wait_for_write as they're helpful for keeping actual library code minimal but until we have a test suite for selectors it's pointless to test them so I've reverted the test_utils.py to not have their tests. I will put the new test suite into a file called test_selectors.py

From this starting point it will be adding a test suite for all selectors and implementing KqueueSelector, EpollSelector and PollSelector which are all much more syscall intense than SelectSelector.

I'd like a review on this code before progressing further.

Lukasa

Ok cool, some feedback inline.

Lukasa · 2016-10-19T16:25:47Z

urllib3/packages/selectors.py

+if hasattr(time, "monotime"):
+    monotonic = time.monotonic
+else:
+    monotonic = time.time


Rather than do if hasattr...else, better to do try...except AttributeError.

Sure, also noticed a mistake using "monotime" instead of "monotonic".

Lukasa · 2016-10-19T16:29:22Z

urllib3/packages/selectors.py

+    result = None
+    while expires is None:
+        current_time = monotonic()
+        if current_time > expires:


With timeout=0 this may fail immediately, because expires is calculated based off an earlier call to monotonic(). We should probably allow one run with timeout=0, so you may want to have current_time updated once before this loop begins (and use it to calculate expires), and then update at the bottom of the loop.

I should note as well that this guarantees that expires is None, and on Python 3 comparing a number to None raises a TypeError.

I will fix this check, the next commit cycle will include tests so I can catch these trivial errors.

Lukasa · 2016-10-19T16:31:57Z

urllib3/packages/selectors.py

+        expires = monotonic() + timeout
+
+    result = None
+    while expires is None:


As noted below, this loop condition cannot possibly be right. Presumably you mean while result is None.

However, this doesn't allow for the possibility that the syscall may return None. Now, I think that's unlikely, but to be defensive against that possibility let's create a sentinel object that we use instead of None.

Wow, complete mistype, can't believe I missed that one. I'll change that. And yes we can use a sentinel object just in case a syscall returns None.

Lukasa · 2016-10-19T16:38:22Z

urllib3/packages/selectors.py

+        self.close()
+
+
+class _BaseSelectorImpl(BaseSelector):


Given that we don't need the utility of an ABC (and indeed given that BaseSelector is not an ABC in this codebase), do we need this separation between BaseSelector and _BaseSelectorImpl?

I was wondering this myself, probably not. I'll merge the two classes. This first revision was mostly to get the basics from selectors down.

Lukasa · 2016-10-19T16:45:22Z

urllib3/packages/selectors.py

+            if events & EVENT_READ:
+                self._readers.add(key.fd)
+            if events & EVENT_WRITE:
+                self._readers.add(key.fd)


This looks wrong.

You are correct.

Lukasa · 2016-10-19T16:47:13Z

urllib3/packages/selectors.py

+elif 'EpollSelector' in globals():
+    DefaultSelector = EpollSelector
+elif 'PollSelector' in globals():
+    DefaultSelector = PollSelector


Obviously all of this jazz raises NameErrors all over the place.

Shouldn't, this is the same method of doing things that's used in selectors.py. I can change to something else though.

sethmlarson · 2016-10-19T16:53:38Z

Thanks for the initial review @Lukasa

sethmlarson · 2016-10-19T17:14:18Z

@Lukasa Bit of information that I gleaned from PEP 475:
"(Note: the selector module already retries on InterruptedError , but it doesn't recompute the timeout yet)" Based on the PEP saying "yet", I assume the selector module should recompute the timeout. Currently with this implementation the same timeout is used on a retry but it would be made to use a timeout based on the initial timeout amount minus how much time was spent selecting before the interrupt. We should probably take a position on this.

I'm in favor of recalculating the timeout time for certain system calls, perhaps have a parameter called recalc_timeout in _syscall_wrapper that will recalculate timeout if it's passed via kwargs or is the last entry in args. Reason being that timeout times will be regular rather than randomly being up to twice as long based on a system interrupt.

Edit: _syscall_wrapper now recalculates the timeout for the syscall if it's told to.

sethmlarson · 2016-10-19T21:51:56Z

So I added a lot of tests (inspired by asyncio's selectors tests) and I've got coverage up to 94% on Python3.5, the only area that isn't covered by a test is the system call catcher, but that'll get covered by Travis. I've also implemented the PollSelector, so now only EpollSelector and KqueueSelector remain.

Would appreciate another review at this point, especially if all tests pass on Travis.

sethmlarson · 2016-11-02T14:10:42Z

@Lukasa Should other PRs and issues regarding this be closed now that this is finalized? So far I've found #589, #685, #700, #998, and maybe #681

Lukasa · 2016-11-02T14:46:25Z

Well identified. Sadly I have no evidence that #681 will be helped by this PR, so we can't close that one.

sethmlarson · 2016-11-02T14:51:53Z

@Lukasa Yeah I didn't know if this work would have any effect on that issue, I can try to recreate it and see what happens later.

sethmlarson · 2016-11-03T15:53:08Z

Will today be the big day? 🎉

Lukasa · 2016-11-03T15:53:56Z

Once we can get a green test run, you bet it will!

sethmlarson · 2016-11-03T19:12:33Z

WHY MUST THAT BUTTON BE THERE??? :(

Lukasa · 2016-11-03T19:35:51Z

Just to punish you I'm afraid.

sethmlarson · 2016-11-03T20:52:36Z

Now it's done after my silly mistake. :)

Lukasa · 2016-11-03T21:12:26Z

Hooray, merged!

Changelog entry for #1001

Version 1.9.1 + merge ba92f65f2cf7ff65eb03ffc42b7ea065da8152ca Fix urllib3/urllib3#1001

Create custom Selectors (Kqueue, Epoll, Poll, and Select) Backport

Changelog entry for urllib3#1001

sethmlarson added 7 commits October 18, 2016 21:55

Add wait to utils

93c779d

Epoll doesn't take non-float

3650f09

Errors return empty list

0da801a

Make test for bad fds

52c1e11

Python 2.6 don't use with assertRaises

1a362da

Fix is_connection_dropped

f46b2fb

Put timeout in is_connection_dropped

a2f58f4

Lukasa mentioned this pull request Oct 19, 2016

Happy Eyeballs Algorithm #1000

Closed

sethmlarson changed the title ~~Kqueue, Epoll, Devpoll, Poll, and Select added to Utils~~ Create custom Selectors (Kqueue, Epoll, Poll, and Select) Backport Oct 19, 2016

Add SelectSelector and syscall wrapper

983a193

Lukasa requested changes Oct 19, 2016

View reviewed changes

sethmlarson added 2 commits October 19, 2016 12:20

Add Lukasas reviewed changes

2aa0447

Improve coverage to 94%

6e9fbc1

This was referenced Nov 2, 2016

Replace select.select with util functions #685

Closed

Add poll, epoll, kevent/kqueue/Replace select.select with util functions #998

Closed

Merge branch 'master' into wait_selectors

9c51ace

sethmlarson closed this Nov 3, 2016

sethmlarson reopened this Nov 3, 2016

Lukasa merged commit ba92f65 into urllib3:master Nov 3, 2016

Lukasa added a commit to Lukasa/urllib3 that referenced this pull request Nov 4, 2016

Changelog entry for urllib3#1001

b4f7bf6

Lukasa added a commit that referenced this pull request Nov 4, 2016

Merge pull request #1024 from Lukasa/changes1001

d1e0520

Changelog entry for #1001

sethmlarson deleted the wait_selectors branch November 7, 2016 22:15

aniapte mentioned this pull request Nov 22, 2016

Necessary to cleanup DefaultSelector in def _wait_for_io_events() ? #1046

Closed

kunteng added a commit to kunteng/requests that referenced this pull request Jan 16, 2017

Upgrade urllib3

551ad3b

Version 1.9.1 + merge ba92f65f2cf7ff65eb03ffc42b7ea065da8152ca Fix urllib3/urllib3#1001

kunteng mentioned this pull request Jan 16, 2017

Upgrade urllib3 kunteng/requests#1

Open

nelljerram mentioned this pull request Jan 25, 2017

AttributeError: 'module' object has no attribute 'epoll' #1104

Closed

RussellLuo mentioned this pull request Feb 13, 2017

filedescriptor out of range in select() while calling message.delete boto/botocore#756

Closed

joguSD mentioned this pull request Jul 21, 2017

use epoll instead of select boto/boto3#1053

Closed

pzwang mentioned this pull request Apr 27, 2018

conda dies on window resize at critical moments conda/conda#7218

Closed

1 task

henriklindstrom mentioned this pull request May 22, 2018

Stop vendoring requests and urllib3 while being backwards compatible boto/botocore#1466

Closed

florimondmanca mentioned this pull request Sep 13, 2020

Using select is causing issues when many files are open encode/httpcore#182

Closed

Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022

Merge pull request urllib3#1001 from SethMichaelLarson/wait_selectors

da81b0d

Create custom Selectors (Kqueue, Epoll, Poll, and Select) Backport

Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022

Changelog entry for urllib3#1001

432996e

Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022

Merge pull request urllib3#1024 from Lukasa/changes1001

dc77c47

Changelog entry for urllib3#1001

Create custom Selectors (Kqueue, Epoll, Poll, and Select) Backport #1001

Create custom Selectors (Kqueue, Epoll, Poll, and Select) Backport #1001

Conversation

sethmlarson commented Oct 19, 2016

Lukasa commented Oct 19, 2016

sethmlarson commented Oct 19, 2016

Lukasa commented Oct 19, 2016

sethmlarson commented Oct 19, 2016 • edited

Lukasa commented Oct 19, 2016

sethmlarson commented Oct 19, 2016

Lukasa commented Oct 19, 2016

sethmlarson commented Oct 19, 2016

sethmlarson commented Oct 19, 2016 • edited

Lukasa commented Oct 19, 2016

sethmlarson commented Oct 19, 2016

Lukasa commented Oct 19, 2016

sethmlarson commented Oct 19, 2016

sethmlarson commented Oct 19, 2016 • edited

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethmlarson commented Oct 19, 2016

sethmlarson commented Oct 19, 2016 • edited

sethmlarson commented Oct 19, 2016

sethmlarson commented Nov 2, 2016

Lukasa commented Nov 2, 2016

sethmlarson commented Nov 2, 2016

sethmlarson commented Nov 3, 2016

Lukasa commented Nov 3, 2016

sethmlarson commented Nov 3, 2016 • edited

Lukasa commented Nov 3, 2016

sethmlarson commented Nov 3, 2016

Lukasa commented Nov 3, 2016

sethmlarson commented Oct 19, 2016 •

edited

sethmlarson commented Oct 19, 2016 •

edited

sethmlarson commented Oct 19, 2016 •

edited

sethmlarson commented Oct 19, 2016 •

edited

sethmlarson commented Nov 3, 2016 •

edited