Skip to content

Fix KeyError crash when a websocket subscription fails - #373

Open
massimiliano1991 wants to merge 1 commit into
bybit-exchange:masterfrom
massimiliano1991:fix/subscription-failure-keyerror
Open

Fix KeyError crash when a websocket subscription fails#373
massimiliano1991 wants to merge 1 commit into
bybit-exchange:masterfrom
massimiliano1991:fix/subscription-failure-keyerror

Conversation

@massimiliano1991

Copy link
Copy Markdown

Problem

When the server rejects a subscription (success: false), _process_subscription_message calls self._pop_callback(topic[0]). But topic is either the raw JSON subscription message (req_id branch) or a topic string (fallback branch), so topic[0] is '{' or the topic's first character — and _pop_callback raises KeyError.

Because KeyError is not one of the disconnection errors that _on_error tolerates, the exception is re-raised and the whole websocket dies. So one rejected subscription (e.g. a mistyped topic) kills the connection with a cryptic KeyError: '{' instead of the error log the code intends to produce.

Reproduce against current master (no network needed):

m = _V5WebSocketManager("test", testnet=False, callback_function=lambda _: None)
m.subscriptions["r1"] = json.dumps({"op": "subscribe", "req_id": "r1", "args": ["kline.1.BTCUSDT"]})
m._set_callback("kline.1.BTCUSDT", lambda _: None)
m._process_subscription_message({"op": "subscribe", "req_id": "r1", "success": False, "ret_msg": "error:handler not found"})
# KeyError: '{'

Two side effects of the same path: the failed subscription stays in self.subscriptions, so a reconnect replays it (and fails again), and the callback entry is never removed, so re-subscribing to the same topic raises "You have already subscribed to this topic".

Fix

On failure, resolve the args list of the stored subscription message, pop each affected callback that exists, and drop the failed subscription from self.subscriptions so a reconnect does not replay it. The success debug log now shows the topics instead of the raw JSON message. No behaviour change on the success path.

Tests

4 regression tests added (failure with req_id, failure without req_id, multi-symbol failure, success path unchanged). Full suite: 48/48 passing.

@kolya-bybit

Copy link
Copy Markdown
Contributor

I am not sure what exactly this PR seeks to achieve. There should be no legitimate workflow where the application ends up listening to the wrong topic, ever. If there is such workflow, it should be fixed by the developer, as it's an incorrect usage of pybit.

@massimiliano1991

Copy link
Copy Markdown
Author

Agreed — a rejected subscribe means the developer did something wrong, and this PR does not try to make that legitimate. It only changes what happens at that moment. Today the failure branch itself crashes: _pop_callback(topic[0]) passes '{' (the first character of the stored JSON message), so instead of the error log this branch intends — with the server's ret_msg telling the developer what to fix — they get KeyError: '{' and the whole connection dies, healthy topics included. The repro in the description runs without network.

If you'd rather fail hard on a rejected subscribe, happy to change the fix to raise a clear exception instead — the goal is only surfacing ret_msg rather than KeyError: '{'.

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.

2 participants