Skip to content
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

test_heartbeat_timeout.py: reproducer for thread-safety issues in HeartbeatListener #390

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

mikebonnet
Copy link
Contributor

Python sockets are not thread-safe. These tests expose an issue where, in the event of a heartbeat timeout, the heartbeat thread closes the socket while the receiver thread created by the transport is reading from it. This can lead to memory corruption and segmentation faults. Related to #389 .

Reset the "disconnected" variable in the on_connected() method, so
wait_on_disconnected() does the right thing when a connection has
reconnected.
…rtbeatListener

Python sockets are not thread-safe. These tests expose an issue where, in the event
of a heartbeat timeout, the heartbeat thread closes the socket while the receiver
thread created by the transport is reading from it. This can lead to memory corruption
and segmentation faults.
@mikebonnet
Copy link
Contributor Author

Example output I get when running these tests locally:

DEBUG    stomp.py:transport.py:191 Received frame: 'heartbeat', headers={}, body=None
INFO     stomp.py:listener.py:466 on_heartbeat
INFO     stomp.py:test_heartbeat_timeout.py:22 Cutting receive_sleep in half
WARNING  stomp.py:listener.py:304 Heartbeat timeout: diff_receive=0.07520760386250913, time=1489045.61509271, lastrec=1489045.539885106
DEBUG    stomp.py:transport.py:390 nothing received, raising CCE
Fatal Python error: Aborted

Current thread 0x00007f69b6cbb640 (most recent call first):
  File "/usr/lib64/python3.10/ssl.py", line 1317 in unwrap
  File "/home/mikeb/opt/github/stomp.py/stomp/transport.py", line 591 in disconnect_socket
  File "/home/mikeb/opt/github/stomp.py/stomp/listener.py", line 307 in __heartbeat_loop
  File "/usr/lib64/python3.10/threading.py", line 946 in run
  File "/usr/lib64/python3.10/threading.py", line 1009 in _bootstrap_inner
  File "/usr/lib64/python3.10/threading.py", line 966 in _bootstrap

Thread 0x00007f69b632b640 (most recent call first):
  File "/usr/lib64/python3.10/socket.py", line 496 in _real_close
  File "/usr/lib64/python3.10/ssl.py", line 1332 in _real_close
  File "/usr/lib64/python3.10/socket.py", line 502 in close
  File "/home/mikeb/opt/github/stomp.py/stomp/transport.py", line 655 in cleanup
  File "/home/mikeb/opt/github/stomp.py/stomp/transport.py", line 358 in __receiver_loop
  File "/usr/lib64/python3.10/threading.py", line 946 in run
  File "/usr/lib64/python3.10/threading.py", line 1009 in _bootstrap_inner
  File "/usr/lib64/python3.10/threading.py", line 966 in _bootstrap

Thread 0x00007f69c5f57740 (most recent call first):
  File "/usr/lib64/python3.10/threading.py", line 320 in wait
  File "/home/mikeb/opt/github/stomp.py/stomp/listener.py", line 366 in wait_on_disconnected
  File "/home/mikeb/opt/github/stomp.py/tests/test_heartbeat_timeout.py", line 51 in test_heartbeat_timeout_reconnect

@jasonrbriggs
Copy link
Owner

FYI, local testing doesn't work for me with this one. Goes into a loop and then eventually dumps core.

@mikebonnet
Copy link
Contributor Author

That’s the point actually. :) The HeartbeatListener closes the socket from outside the main thread, which leads to memory corruption and core dumps. This test is exposing the problem, I don’t yet have a good solution. No need to merge this yet, I just wanted to call attention to it. Open to suggestions for a fix.

@jasonrbriggs
Copy link
Owner

That’s the point actually. :)

LOL, yeah sorry I suddenly realised that after I commented and then went to bed. It was a late night senile moment...

I'll give it some further as well. Not sure how to resolve either...

@s17b2-voroneckij
Copy link

@jasonrbriggs, are there any plans for fixing this? Or at least a way to overcome this issue?

@jasonrbriggs
Copy link
Owner

No plans at the moment. I started working on a rewrite using asyncio (https://github.com/jasonrbriggs/stomp.py/tree/asyncio-experimental), which might have been a solution, but never got it working completely, nor had the time to finish it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants