-
Notifications
You must be signed in to change notification settings - Fork 28
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
Drop py2! #34
Conversation
Auto-reconnects should be handled on the connection side and not in an event loop handler where it could block other tasks. Rename the event loop's connection variable to just `_con`; we don't need separate transmit and receive connections any more since dropping SWIG.
Add back in support at the connection layer using coroutines such that reconnect logic never blocks the event loop. The protocol now calls back and schedules a reconnect coroutine whenever `connection_lost()` is called and `autorecon` is set. Fixes #32
@vodik @moises-silva ping ping! |
2e22d33
to
ea41e13
Compare
README.rst
Outdated
|
||
What's included? | ||
---------------- | ||
- A slew of `built-in apps`_ |
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.
Double space
setup.py
Outdated
|
||
reqs = ['click'] | ||
|
||
with open('README.rst') as f: | ||
readme = f.read() |
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.
Now you're 3 only, put encoding='utf-8'
for maximum comparability across possibly broken Python environments without a correct LANG set (I've seen it happen).
switchio/connection.py
Outdated
|
||
await asyncio.wait_for( | ||
loop.create_connection(lambda: prot, host, port), | ||
timeout=0.1) |
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.
Is that maybe a bit fast?
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.
Turns out, no, otherwise it wouldn't work but that's in local testing of course ;)
Good point that this might be a problem when making remote attempts.
switchio/connection.py
Outdated
|
||
for i in range(count): | ||
try: | ||
await try_connect(host, port, password, prot, loop, log) |
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.
Loop inside a loop?
Honestly, might be easier to just wrap this whole thing in an async_timeout
, use an absolute timeout instead of faking it with iterations, and just do a while loop:
with async_timeout(10):
while True:
await try_connect(...)
break
Error/timeout handling missing, obviously
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.
Yes, try_reconnect
is a longer running task that attempts to call the normal connection logic asynchronously when the connection drops.
Which part are you saying to wrap? try_connect
just tries 5 times before baling whereas try_reconnect
is waiting up to autorecon
seconds.
super().__init__(*args, **kwargs) | ||
class EventLoop(object): | ||
'''Processes ESL events using a background (thread) ``asyncio`` event loop | ||
and one ``aioesl`` connection. |
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.
You sure you need a separate loop for this? Or that left for investigation after dropping py2?
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.
Not sure what you mean.
Is it the EventLoop
name that's got you wondering?
This is a proactor / scheduler but also one big loop which processes events.
''' | ||
# retrieve cached event/blocker if possible | ||
event = mp.Event() if not self._blockers else self._blockers.pop() | ||
waiters = self._sess2waiters.setdefault(sess, {}) # sess -> {vars: ..} |
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.
collections.defaultdict might be a good fit here.
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.
This is going to get dropped entirely and replaced with something similar like Session.recv()
(Session.waitfor(var='blah')
maybe?) which returns a future that's used for inter-coroutine comms (ICC?) heh?
Hmm although I guess the point of this was actually for inter-thread/proc comms..
So maybe yeah I will change it.
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.
Yeah gonna defer this until solving #12
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.
Looks mostly good, just a few things to keep in mind/improve.
There's a bit more come in the form of solving a couple issues that are related to the lingering legacy support.
Let me know what you think of the initial draft!