-
Notifications
You must be signed in to change notification settings - Fork 761
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
fix for stack growth on reconnect #854
Changes from 31 commits
71a6b66
0a090e6
934f665
0b14e89
255dcae
93fa6a8
cb73493
3ccecbd
58d4b5f
7514d17
85342a8
218e33c
a90f540
754cf66
56f5f12
8b6ae4b
11b2f69
2d9f27b
8916d4f
6572125
23f3287
d905d23
f6088c9
07cfd26
c588d48
2b10daf
2840403
64d48c5
592ab59
f5b1fc7
baa3365
8537e50
99ff287
d1a843d
d9dc788
fbcc7e9
8d9a08a
adca14d
2f438fc
6077fe5
f1415ba
e716454
5d318ff
c373193
f8596f2
a304b7a
1f0bcab
c35700a
521f793
82f6d97
b0817c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,19 @@ def __init__(self, app, ping_timeout): | |
self.app = app | ||
self.ping_timeout = ping_timeout | ||
|
||
def timeout(self, seconds, callback): | ||
time.sleep(seconds) | ||
callback() | ||
|
||
def reconnect(self, seconds, reconnector): | ||
try: | ||
while True: | ||
_logging.info("reconnect() - retrying in %s seconds [%s frames in stack]" % (seconds, len(inspect.stack()))) | ||
time.sleep(seconds) | ||
reconnector(reconnecting=True) | ||
except KeyboardInterrupt as e: | ||
pass | ||
|
||
|
||
class Dispatcher(DispatcherBase): | ||
""" | ||
|
@@ -56,10 +69,6 @@ def read(self, sock, read_callback, check_callback): | |
check_callback() | ||
sel.close() | ||
|
||
def timeout(self, seconds, callback): | ||
time.sleep(seconds) | ||
callback() | ||
|
||
|
||
class SSLDispatcher(DispatcherBase): | ||
""" | ||
|
@@ -96,14 +105,18 @@ def __init__(self, app, ping_timeout, dispatcher): | |
self.app = app | ||
self.ping_timeout = ping_timeout | ||
self.dispatcher = dispatcher | ||
dispatcher.signal(2, dispatcher.abort) # keyboard interrupt | ||
|
||
def read(self, sock, read_callback, check_callback): | ||
self.dispatcher.read(sock, read_callback) | ||
self.ping_timeout and self.dispatcher.timeout(self.ping_timeout, check_callback) | ||
self.ping_timeout and self.timeout(self.ping_timeout, check_callback) | ||
|
||
def timeout(self, seconds, callback): | ||
self.dispatcher.timeout(seconds, callback) | ||
|
||
def reconnect(self, seconds, reconnector): | ||
self.timeout(seconds, reconnector) | ||
|
||
|
||
class WebSocketApp: | ||
""" | ||
|
@@ -331,7 +344,7 @@ def teardown(close_frame=None): | |
# Finally call the callback AFTER all teardown is complete | ||
self._callback(self.on_close, close_status_code, close_reason) | ||
|
||
def setSock(): | ||
def setSock(reconnecting=False): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's so that on line 370 it doesn't call handleDisconnect() again if it's already "reconnecting". That's actually the heart of this pull request - in synchronous (default) mode, the stack grows if handleDisconnect() is called (by way of a couple intermediate functions) recursively. Thus, when DispatcherBase.reconnect() (line 51) calls reconnector() (which is setSock()), it passes reconnecting=True, so that if the connection fails it exits setSock(), returns to DispatcherBase.reconnect() (popping the setSock() frame off the stack), and tries again after the wait interval. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And of course I'm open to changing "reconnecting" if there's a better name for that kwarg :) |
||
self.sock = WebSocket( | ||
self.get_mask_key, sockopt=sockopt, sslopt=sslopt, | ||
fire_cont_frame=self.on_cont_message is not None, | ||
|
@@ -352,18 +365,21 @@ def setSock(): | |
|
||
_logging.warning("websocket connected") | ||
dispatcher.read(self.sock.sock, read, check) | ||
except (Exception, ConnectionRefusedError, KeyboardInterrupt, SystemExit) as e: | ||
handleDisconnect(e) | ||
except (WebSocketConnectionClosedException, ConnectionRefusedError, KeyboardInterrupt, SystemExit, Exception) as e: | ||
_logging.error("%s - %s" % (e, reconnect and "reconnecting" or "goodbye")) | ||
reconnecting or handleDisconnect(e) | ||
|
||
def read(): | ||
if not self.keep_running: | ||
return teardown() | ||
|
||
try: | ||
op_code, frame = self.sock.recv_data_frame(True) | ||
except WebSocketConnectionClosedException as e: | ||
_logging.error("WebSocketConnectionClosedException - %s" % (reconnect and "reconnecting" or "goodbye")) | ||
return handleDisconnect(e) | ||
except (WebSocketConnectionClosedException, KeyboardInterrupt) as e: | ||
if custom_dispatcher: | ||
return handleDisconnect(e) | ||
else: | ||
raise e | ||
if op_code == ABNF.OPCODE_CLOSE: | ||
return teardown(frame) | ||
elif op_code == ABNF.OPCODE_PING: | ||
|
@@ -404,10 +420,11 @@ def handleDisconnect(e): | |
raise | ||
if reconnect and not isinstance(e, KeyboardInterrupt): | ||
_logging.info("websocket disconnected (retrying in %s seconds) [%s frames in stack]" % (reconnect, len(inspect.stack()))) | ||
dispatcher.timeout(reconnect, setSock) | ||
dispatcher.reconnect(reconnect, setSock) | ||
else: | ||
teardown() | ||
|
||
custom_dispatcher = bool(dispatcher) | ||
dispatcher = self.create_dispatcher(ping_timeout, dispatcher, not not sslopt) | ||
|
||
if ping_interval: | ||
|
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.
Should this be
self.timeout(seconds, callback)
instead ofself.dispatcher.timeout(seconds, callback)
?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 - it's defined on line 114