Conversation
WalkthroughThis pull request implements in-flight request tracking for UDP handling to prevent overlapping processing of concurrent requests from the same client IP. It adds tracking mechanisms, a helper method for cleanup, modified request submission logic, and corresponding test updates. Changes
Sequence DiagramsequenceDiagram
participant Client as UDP Client
participant Server as UDP Server
participant Tracking as In-Flight Tracker
participant Executor as Thread Executor
participant Handler as Request Handler
Client->>Server: Send UDP request from IP X
Server->>Tracking: Check if IP X in in_flight_senders
alt Request not in-flight
Tracking-->>Server: Not tracked
Server->>Tracking: Add IP X to in_flight_senders
Server->>Executor: Submit request with tracking wrapper
Executor->>Handler: Process request
Handler-->>Executor: Complete
Executor->>Tracking: Remove IP X from in_flight_senders
Executor-->>Server: Response ready
else Request already in-flight
Tracking-->>Server: Already tracked (duplicate)
Server-->>Client: Discard duplicate request
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@shelly/shelly.py`:
- Around line 136-146: The code adds addr[0] to the _in_flight_senders set
before calling self._executor.submit(self._handle_request_with_tracking, sock,
data, addr) which can raise and leak the sender; modify the block around
_executor.submit so that the submit call is wrapped in a try/except (catching
exceptions like RuntimeError) and on any exception remove addr[0] from
_in_flight_senders inside the same _in_flight_lock scope (or re-acquire it) to
ensure the sender is unblocked; reference the existing symbols _in_flight_lock,
_in_flight_senders, _executor.submit and _handle_request_with_tracking when
applying the change.
🧹 Nitpick comments (1)
shelly/shelly_udp_test.py (1)
57-65: Timing-sensitive test — consider widening the recv timeout margin.The
0.3ssocket timeout leaves little headroom above the0.2sSlowPowermetersleep. On a loaded CI runner, the first response could arrive after the timeout, causinglen(responses) == 0and a flaky failure. Bumping the timeout to0.5s(and the duration assertion to< 1.0) would keep the test meaningful while reducing flakiness.
| with self._in_flight_lock: | ||
| if addr[0] in self._in_flight_senders: | ||
| logger.debug( | ||
| "Discarding request from %s; previous request still in progress", | ||
| addr[0], | ||
| ) | ||
| continue | ||
| self._in_flight_senders.add(addr[0]) | ||
| self._executor.submit( | ||
| self._handle_request_with_tracking, sock, data, addr | ||
| ) |
There was a problem hiding this comment.
Sender leaks in _in_flight_senders if submit raises.
If self._executor.submit(...) throws (e.g., RuntimeError after executor shutdown), addr[0] was already added at Line 143 but the tracking wrapper never runs, so discard in the finally block is never reached. That sender is permanently blocked.
Wrap the submit in a try/except that removes the sender on failure:
Proposed fix
self._in_flight_senders.add(addr[0])
- self._executor.submit(
- self._handle_request_with_tracking, sock, data, addr
- )
+ try:
+ self._executor.submit(
+ self._handle_request_with_tracking, sock, data, addr
+ )
+ except RuntimeError:
+ with self._in_flight_lock:
+ self._in_flight_senders.discard(addr[0])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with self._in_flight_lock: | |
| if addr[0] in self._in_flight_senders: | |
| logger.debug( | |
| "Discarding request from %s; previous request still in progress", | |
| addr[0], | |
| ) | |
| continue | |
| self._in_flight_senders.add(addr[0]) | |
| self._executor.submit( | |
| self._handle_request_with_tracking, sock, data, addr | |
| ) | |
| with self._in_flight_lock: | |
| if addr[0] in self._in_flight_senders: | |
| logger.debug( | |
| "Discarding request from %s; previous request still in progress", | |
| addr[0], | |
| ) | |
| continue | |
| self._in_flight_senders.add(addr[0]) | |
| try: | |
| self._executor.submit( | |
| self._handle_request_with_tracking, sock, data, addr | |
| ) | |
| except RuntimeError: | |
| with self._in_flight_lock: | |
| self._in_flight_senders.discard(addr[0]) |
🤖 Prompt for AI Agents
In `@shelly/shelly.py` around lines 136 - 146, The code adds addr[0] to the
_in_flight_senders set before calling
self._executor.submit(self._handle_request_with_tracking, sock, data, addr)
which can raise and leak the sender; modify the block around _executor.submit so
that the submit call is wrapped in a try/except (catching exceptions like
RuntimeError) and on any exception remove addr[0] from _in_flight_senders inside
the same _in_flight_lock scope (or re-acquire it) to ensure the sender is
unblocked; reference the existing symbols _in_flight_lock, _in_flight_senders,
_executor.submit and _handle_request_with_tracking when applying the change.
1 participant
Motivation
Description
self._in_flight_sendersprotected byself._in_flight_lockinshelly/shelly.py._handle_request_with_trackingto call the existing handler and always remove the sender from the in-flight set after handling completes.udp_serverto check and add the sender to the in-flight set before submitting work and to log and discard overlapping requests from the sameaddr[0].shelly/shelly_udp_test.pyto use aSlowPowermeter, set a client timeout withclient.settimeout(0.3), collect responses after issuing concurrent requests, and assert only one response is received during a throttled overlap.## Nextsection ofCHANGELOG.md.Testing
pytest -q shelly/shelly_udp_test.pyand the test passed.Codex Task
Summary by CodeRabbit