fix for stack growth on reconnect #854

bubbleboy14 · 2022-08-26T18:14:28Z

previously, using the default (synchronous) dispatcher, reconnecting would cause the stack to grow.

here's the fix.

…d except TimeoutError - raise WebSocketTimeoutException

…ent compatibility): WrappedDispatcher (for use with generic event dispatchers such as pyevent and rel); create_dispatcher() accepts dispatcher kwarg (default None), and if it is specified, returns a WrappedDispatcher; use create_dispatcher() (passing specified dispatcher if any) every time (regardless of dispatcher specification)

…ndleDisconnect()

…fault False] is True) to setSock() (prevents those lines from running on ConnectionRefusedError)

… mode); added stack frame count to disconnect (warning) log; grossly oversimplified ;)

…edDispatcher.timeout()

… to SSLDispatcher)

…er (uses timeout()); setSock() reconnecting (default False) kwarg - if reconnecting, skip handleDisconnect(); handleDisconnect() calls dispatcher.reconnect()

aantn · 2022-09-02T09:51:18Z

Does the stack grow in version 1.3.3 as well or only 1.4.0?

ollo69 · 2022-09-03T10:17:37Z

Hi, I don't know about stack grow, but for sure 1.4.0 is introducing a bug when used with SSLDispatcher that is causing error AttributeError: 'SSLDispatcher' object has no attribute 'timeout' during disconnect. This was also fixed in PR #855 that was closed because was containing same code of this PR. I think this should be fixed with priority, doesn't it?

aantn · 2022-09-03T13:24:58Z

We've rolled back to version 1.3.3 - the upgrade to version 1.4.0 broke something for us. Our server might be responsible too, but the client behaviour in 1.3.3 didn't cause any trouble.

…tered with setReconnect()) to preserve old (non-reconnecting) default behavior for existing integrations

…orking...)

bubbleboy14 · 2022-09-03T19:40:20Z

websocket/tests/test_app.py

@@ -6,6 +6,7 @@
 import websocket as ws
 import ssl
 import unittest
+import rel


This is to fix the testRunForeverDispatcher() test -- the 'dispatcher="Dispatcher"' previously on line 101 isn't correct.

I'm not sure if lines 102-103 will actually make the test work, I don't know how to add rel to the test dependencies.

Do these tests work in the master branch? As far as I can tell, testRunForeverDispatcher() at least doesn't seem like it could have.

…default

…and testRunForeverDispatcher() (also not working previously afaik)

…to True before calling on_error callback)

codecov · 2022-09-03T20:23:45Z

Codecov Report

Merging #854 (b0817c0) into master (6117159) will decrease coverage by 1.66%.
The diff coverage is 67.01%.

@@            Coverage Diff             @@
##           master     #854      +/-   ##
==========================================
- Coverage   84.53%   82.87%   -1.67%     
==========================================
  Files          13       13              
  Lines        1293     1331      +38     
  Branches      274      278       +4     
==========================================
+ Hits         1093     1103      +10     
- Misses        128      153      +25     
- Partials       72       75       +3

Impacted Files	Coverage Δ
websocket/_core.py	`80.95% <ø> (-0.96%)`	⬇️
websocket/_app.py	`75.89% <64.70%> (-9.22%)`	⬇️
websocket/_logging.py	`82.05% <75.00%> (+0.97%)`	⬆️
websocket/_http.py	`86.32% <83.33%> (ø)`
websocket/__init__.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

bubbleboy14 · 2022-09-03T20:53:31Z

Hello all. @engn33r, all the tests are passing now, with the exception of the Codecov tests (which indicate decreased coverage), which makes sense, because there aren't tests yet for the new code.

I disabled the testRunForeverDispatcher() test because it can only work with rel, but I don't know how to add rel to the test dependencies. Also, I don't think this test is working in master.

I propose we merge this in and call it 1.4.1 for @aantn and @ollo69 and the other ha-samsungtv-smart people.

engn33r · 2022-09-04T19:51:32Z

Nice work @bubbleboy14! I will merge and create a new release. With all the effort you've put in lately, send me an email at websocket.client.email [at] gmail.com if you're interested in joining as a co-maintainer.

maksimu · 2022-09-06T23:36:48Z

after upgrading to 1.4.1 i'm getting following error:

++Sent raw: b'\x81\xaa{\xe3\x0c\x01\x00\xc1eeY\xd9,#\x18\x8ccj\x12\x86.-[\xc1|`\x02\x8fc`\x1f\xc16!Y\xa1MS>\xbc^N.\xb7ISY\x9e'
++Sent decoded: fin=1 opcode=1 data=b'{"id": "cok", "payload": "NONE"}'
websocket connected
'socket' object has no attribute 'pending' - goodbye
Warning: Output is not to a terminal (fd=1).

Things work as expected with version 1.3.3

wondering from where this error is coming from?

maksimu · 2022-09-07T00:02:37Z

Actually found that the error is coming from THIS line

bubbleboy14 · 2022-09-07T01:58:08Z

Actually found that the error is coming from THIS line

Funny thread about sock.pending(): https://bugs.python.org/issue21430

These people in 2014 were saying "this function should not be used" - maybe it got removed in a recent version of Python.

Feel up to patching this @maksimu?

maksimu · 2022-09-07T16:10:40Z

Funny thread about sock.pending(): https://bugs.python.org/issue21430

Funny indeed, and puzzling at the same time.

maybe it got removed in a recent version of Python

I doubt, I'm on Python 3.10 and it works fine with websocket-client version 1.3.3 but when I upgrade it to 1.4.0+ I'm getting this error.

Feel up to patching this @maksimu?

Python is not my strongest language, just maintaining things here and there, so I would be better if someone more knowledgeable could look at what's going on and patch it up.

bubbleboy14 · 2022-09-07T22:29:59Z

@maksimu do you have a concise test case for this? My wss tests aren't failing.

bubbleboy14 and others added 30 commits December 7, 2021 15:44

rel example

71a6b66

tweaked rel example for linter

0a090e6

added rel note to examples.rst

934f665

slightly more compact example

0b14e89

added example header

255dcae

matched wording

93fa6a8

Merge remote-tracking branch 'upstream/master'

cb73493

_socket.recv(): _recv() except socket.error - changed or to and; adde…

3ccecbd

…d except TimeoutError - raise WebSocketTimeoutException

Add clarifying comment, rerun CI tests

7514d17

Add space to make linter happy

85342a8

merged upstream

218e33c

Merge branch 'master' of github.com:bubbleboy14/websocket-client

a90f540

working reconnect

754cf66

rmed logs

56f5f12

added _logging.warning() disconnected/reconnected notifications to ha…

8b6ae4b

…ndleDisconnect()

moved connect notification and dispatcher.read() (if doread kwarg [de…

11b2f69

…fault False] is True) to setSock() (prevents those lines from running on ConnectionRefusedError)

run_forever(): reconnect kwarg now specifies sleep() time (defualt 5)

2d9f27b

handleDisconnect(): fixed log msg

8916d4f

run_forever() refactor: stabilized stack frame count (at least in rel…

6572125

… mode); added stack frame count to disconnect (warning) log; grossly oversimplified ;)

dispatcher simplification via DispatcherBase and DispatcherBase/Wrapp…

23f3287

…edDispatcher.timeout()

_logging: info(); enableTrace() supports level kwarg (default "DEBUG")

d905d23

handleDisconnect() uses info() log

f6088c9

Fix linting errors

07cfd26

merged in upstream

c588d48

moved timeout() from Dispatcher to DispatcherBase (thus also applying…

2b10daf

… to SSLDispatcher)

reconnect()s for DispatcherBase (uses while loop) and WrappedDispatch…

2840403

…er (uses timeout()); setSock() reconnecting (default False) kwarg - if reconnecting, skip handleDisconnect(); handleDisconnect() calls dispatcher.reconnect()

custom_dispatcher switch in handleDisconnect()

64d48c5

merged

592ab59

WrappedDispatcher constructor registers keyboard interrupt signal

f5b1fc7

bubbleboy14 added 5 commits August 30, 2022 10:11

linter line

8d9a08a

added rel to setup extras_require{test}[]

adca14d

adjusted testRunForeverDispatcher() to use rel (including dispatch())

2f438fc

setup: moved rel dep from extras_require{test}[] to tests_require[]

6077fe5

meh trying install_requires[] (tests_require[] depped??)

f1415ba

ollo69 mentioned this pull request Sep 2, 2022

Integration working bad last few days ollo69/ha-samsungtv-smart#178

Closed

bubbleboy14 added 3 commits September 3, 2022 12:30

set RECONNECT (run_forever() reconnect kwarg default) to 0 (can be al…

e716454

…tered with setReconnect()) to preserve old (non-reconnecting) default behavior for existing integrations

rmed rel from install_requires[] (only added for tests, and was not w…

5d318ff

…orking...)

test_app: rmed ws.setReconnect(0) (0 is new default)

c373193

bubbleboy14 commented Sep 3, 2022

View reviewed changes

bubbleboy14 added 7 commits September 3, 2022 12:50

run_forever() reconnect->RECONNECT fallback in func instead of kwarg …

f8596f2

…default

test_app: disabled rel import (unsure how to set up test dependency) …

a304b7a

…and testRunForeverDispatcher() (also not working previously afaik)

linter fixes

1f0bcab

linter comment spaces

c35700a

run_forever() returns False to pass testRunForeverTeardownCleanExit test

521f793

run_forever() returns False unless error (handleDisconnect() changes …

82f6d97

…to True before calling on_error callback)

rval->self.has_errored

b0817c0

engn33r merged commit 3baacda into websocket-client:master Sep 4, 2022

engn33r mentioned this pull request Sep 7, 2022

'socket' object has no attribute 'pending' #857

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for stack growth on reconnect #854

fix for stack growth on reconnect #854

bubbleboy14 commented Aug 26, 2022

aantn commented Sep 2, 2022

ollo69 commented Sep 3, 2022

aantn commented Sep 3, 2022 •

edited

bubbleboy14 Sep 3, 2022

codecov bot commented Sep 3, 2022

bubbleboy14 commented Sep 3, 2022

engn33r commented Sep 4, 2022

maksimu commented Sep 6, 2022 •

edited

maksimu commented Sep 7, 2022

bubbleboy14 commented Sep 7, 2022

maksimu commented Sep 7, 2022 •

edited

bubbleboy14 commented Sep 7, 2022

fix for stack growth on reconnect #854

fix for stack growth on reconnect #854

Conversation

bubbleboy14 commented Aug 26, 2022

aantn commented Sep 2, 2022

ollo69 commented Sep 3, 2022

aantn commented Sep 3, 2022 • edited

bubbleboy14 Sep 3, 2022

Choose a reason for hiding this comment

codecov bot commented Sep 3, 2022

Codecov Report

bubbleboy14 commented Sep 3, 2022

engn33r commented Sep 4, 2022

maksimu commented Sep 6, 2022 • edited

maksimu commented Sep 7, 2022

bubbleboy14 commented Sep 7, 2022

maksimu commented Sep 7, 2022 • edited

bubbleboy14 commented Sep 7, 2022

aantn commented Sep 3, 2022 •

edited

maksimu commented Sep 6, 2022 •

edited

maksimu commented Sep 7, 2022 •

edited