add pinger routine

[kenanfarukcakir]

No description provided.

[devin-ai-integration]

PR Review: Add Pinger Routine

Overall this looks solid — the approach of application-level pings (JSON {"type": "ping"}) on a background thread/task is correct for the Lighter protocol, and the shutdown/cleanup logic is well-structured. A few issues to flag:

---

Issues

1. Unused import time
time is imported but never used anywhere in the file. Should be removed.

2. Exception swallowing — silent behavioral change
Both run() and run_async() now wrap everything in try/except Exception and just print() the error. Previously, exceptions propagated to the caller. This silently changes behavior:

• handle_unhandled_message raises Exception(f"Unhandled message: {message}") — now silently caught and printed.
• User-provided callbacks (on_order_book_update, on_account_update) that raise exceptions — also silently caught.
• Connection errors — silently caught.

This could mask real bugs. Consider either re-raising after cleanup, or separating "expected" close/disconnect errors from unexpected ones. At minimum, callers who previously relied on exceptions propagating from run()/run_async() will silently get different behavior.

3. Minor race condition in run() finally block
The ordering is:

finally:
    stop_event.set()
    self.ws = None          # ← clears ws
    if ping_thread:
        ping_thread.join()  # ← ping thread may still be mid-send

The ping thread could be between the if self.ws check and self.ws.send() when self.ws is set to None, causing an AttributeError. It'd be caught by _ping_loop's except, so it won't crash — but it's messy. Safer ordering:

finally:
    stop_event.set()
    if ping_thread:
        ping_thread.join(timeout=1)
    self.ws = None  # clear after thread has exited

Note: the async version already does this correctly (cancels task → gathers → then clears self.ws).

---

Looks Good

• Context manager usage: Upgrading from bare connect() to with connect() as ws: / async with connect_async() as ws: is a good improvement for proper cleanup.
• Pong handling: Correctly added "pong" as a no-op in both sync and async message handlers — necessary since the client now initiates pings.
• Clean shutdown: stop_event + daemon=True thread (sync) and task.cancel() + asyncio.gather(return_exceptions=True) (async) are both correct patterns.
• asyncio.CancelledError handling: Properly caught in _ping_loop_async so it exits cleanly on cancellation.
• Configurable interval: ping_interval=30 as a constructor param with a sensible default is nice.
• stop_event.wait(self.ping_interval) in the sync loop is better than time.sleep since it's immediately interruptible when stop is signaled.

---

Summary

The core ping/pong logic is correct and will work. The main concern is #2 (exception swallowing) which is a behavioral change that could hide bugs from callers. #1 is trivial cleanup, and #3 is a minor race that won't crash but should be tidied up for correctness.

[devin-ai-integration]

Re-Review after d01f071

All three issues from my initial review are resolved:

1. Unused time import — Removed. ✔
2. Exception swallowing — except Exception blocks removed. Both run() and run_async() now use try/finally only, so exceptions propagate to callers exactly as before this PR. ✔
3. Race condition in sync run() finally block — self.ws = None now happens after ping_thread.join(timeout=1), consistent with the async version. ✔

Final state looks clean:

• run() (sync): try/finally → stop_event.set() → join(timeout=1) → self.ws = None — correct order.
• run_async(): try/finally → stop_event.set() → cancel() → gather(return_exceptions=True) → self.ws = None — correct order.
• _ping_loop (sync): stop_event.wait() is interruptible, checks both self.ws and stop_event before send, catches send errors — good.
• _ping_loop_async: asyncio.sleep + CancelledError handling, checks self.ws and stop_event before send — good.
• Pong handling: Both sync/async message handlers correctly treat "pong" as a no-op.
• No behavioral regressions: The only additions are the ping thread/task and pong handling; all existing message handling, callbacks, and exception propagation remain unchanged.

LGTM — no further issues. Ready to merge.