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

Fix reactor stop hang #179

Merged
merged 3 commits into from
Dec 14, 2022

Conversation

EliaOnceAgain
Copy link
Contributor

@EliaOnceAgain EliaOnceAgain commented Oct 28, 2022

Twisted reactor not exiting cleanly. Encountered while running pytest, which does not exit after all tests are finished. Reproduction steps below.

Ubuntu 22.04.1 LTS
Python 3.10.6
binance-connector==1.18.0
Twisted==22.8.0
pytest==7.2.0

From: Invoking Twisted From Other Threads

Methods within Twisted may only be invoked from the reactor thread unless otherwise noted... This means that if you start a thread and call a Twisted method, you might get correct behavior… or you might get hangs, crashes, or corrupted data. So don’t do it.

Test case:

import pytest
from binance.spot import Spot
from binance.websocket.spot.websocket_client import SpotWebsocketClient as WebsocketClient


class TestBinanceAPI:
    @pytest.fixture(scope="class")
    def ws_client(self):
        client = WebsocketClient()
        client.start()
        yield client
        client.stop()

    def test_hello_world(self, ws_client):
        assert ws_client

Expected outcome:
Test finishes successfully, pytest exists gracefully at the end.

Actual outcome:
Test finishes successfully, pytest hangs indefinitely

Pre fix output

> /my-path/tests/test_wsclient.py(12)ws_client()
-> client.stop()
> /my-venv/lib/python3.10/site-packages/binance/websocket/websocket_client.py(15)stop()
-> reactor.stop()

(Pdb) ll
 10         def stop(self):
 11             try:
 12                 self.close()
 13             finally:
 14                 import pdb; pdb.set_trace()
 15  ->            reactor.stop()
(Pdb) reactor._stopped
False
(Pdb) reactor.running
True
(Pdb) n
--Return--
> /my-env/lib/python3.10/site-packages/binance/websocket/websocket_client.py(15)stop()->None
-> reactor.stop()
(Pdb) reactor.running
True <<<<<<< Still true
(Pdb) reactor._stopped
True <<<<<<< Changed to true, yet still running
(Pdb) continue

At this point, pytest hangs, never exits or gives up control of the command line. Last printed logline:
============================================================================= 1 passed in 124.56s (0:02:04) ==============================================================================

Post fix output

-> reactor.callFromThread(reactor.stop)
(Pdb) ll
 10         def stop(self):
 11             try:
 12                 self.close()
 13             finally:
 14                 import pdb; pdb.set_trace()
 15                 # reactor.stop()
 16  ->             reactor.callFromThread(reactor.stop)
(Pdb) reactor._stopped
False
(Pdb) reactor.running
True
(Pdb) n
--Return--
> /my-env/lib/python3.10/site-packages/binance/websocket/websocket_client.py(16)stop()->None
-> reactor.callFromThread(reactor.stop)
(Pdb) reactor._stopped
True
(Pdb) reactor.running
False <<<<< correctly changed
(Pdb) continue

At this point, pytest finishes successfully, exits gracefully, and returns command line control to the caller.

@2pd 2pd added the reviewing reviewing the issue label Nov 4, 2022
@aisling-2 aisling-2 deleted the branch binance:v2.0.0-rc3 November 7, 2022 02:27
@aisling-2 aisling-2 closed this Nov 7, 2022
@aisling-2 aisling-2 reopened this Nov 7, 2022
@aisling-2 aisling-2 changed the base branch from rc-1.18.0 to rc-1.19.0 November 7, 2022 04:36
@2pd 2pd self-requested a review December 12, 2022 03:52
@jonte-z jonte-z changed the base branch from rc-1.19.0 to v2.0.0-rc3 December 13, 2022 05:46
@jonte-z jonte-z merged commit 33364b1 into binance:v2.0.0-rc3 Dec 14, 2022
@jonte-z jonte-z removed the reviewing reviewing the issue label Dec 14, 2022
@EliaOnceAgain EliaOnceAgain deleted the elia/fix-reactor-stop-hang branch January 3, 2024 23:00
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.

None yet

4 participants