testserver: unique notification handles + survive bad input - #539
Conversation
Three robustness fixes to the AdvancedHandler test server, found while using it to exercise an EPICS ADS driver's bulk-read and notification paths against the full symbol set of a real PLC project. 1. PLCVariable.register_notification handed out colliding handles. notification_count is a class attribute, but `self.notification_count += 1` rebinds it per-instance, so the first notification registered on every variable returns the same handle (10), the second 11, and so on. A client subscribed to more than one variable then cannot tell the notifications apart. Increment the shared class counter so handles are globally unique, as real TwinCAT returns them. 2. AdsClientConnection.run called recvfrom() with no guard. A client killed mid-request drops the connection abruptly, raising ConnectionResetError and crashing the handler thread, which leaves the server unable to serve the next connection. Treat the reset as a disconnect. 3. A command handler that raised killed the connection's handler thread, leaving the client's later requests unanswered until they time out. handle_read_write does write_data.decode() on the read-write payload, which raises UnicodeDecodeError when the payload is not a UTF-8 symbol name (e.g. a binary SUMUP batch), and get_variable_by_name raises on an unknown symbol. Catch handler exceptions at the dispatch, log them, and return an ADS device error instead.
Coverage Report for CI Build 28048453040Coverage increased (+0.3%) to 93.866%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions24 previously-covered lines in 4 files lost coverage. Coverage Stats
💛 - Coveralls |
Add tests exercising the two error branches introduced in this branch: AdvancedHandler converting a raising command handler into an ADS device error, and AdsClientConnection surviving a client RST mid-request. Restores the coverage the robustness commit dropped by adding untested branches.
4c5a6d9 to
7056f37
Compare
|
Added some test for the feature and fixes mentions in the PR |
stlehmann
left a comment
stlehmann
left a comment
There was a problem hiding this comment.
Thanks for tracking these down — the root causes are all correctly diagnosed (class-vs-instance counter, unguarded recvfrom, uncaught handler exception), and the fixes match real TwinCAT behavior. A few smaller things worth a look before merge:
1. advanced_handler.py — SUMUP_WRITE (~L357-378) + new catch-all (~L497)
One bad sub-request in a SUMUP_WRITE batch now returns a single 0x0700 batch error — but earlier sub-writes (and their notification callbacks) already landed before the exception hit. Real ADS reports one error code per sub-request: adsSyncReadWriteReqEx2 already allocates index_offset * 4 response bytes for exactly this, and adsSumWriteBytes/adsSumRead already parse it that way. The SUMUP_WRITE branch already builds a same-shaped array, just hardcoded to all-zeros — filling it with real per-item codes on a per-iteration catch would match both the protocol and pyads' own client parsing.
2. testserver.py (~L197-202)
The new except (ConnectionResetError, OSError) around recvfrom swallows the error with no log line, unlike the dispatch-level catch in advanced_handler.py which calls logger.exception(...). A real socket bug would now vanish silently — worth a log line here too.
3. Minor, same line — ConnectionResetError is already an OSError subclass, so the tuple is redundant with just OSError.
4. Minor, advanced_handler.py (~L502) — 0x0700 is correct (1792 = ADS "device error" class, per errorcodes.py), just unnamed — pyads has no named error-code constants anywhere yet, and the file's existing "unknown command" line has the same bare-literal style. Worth a follow-up constant, not a blocker.
Generated by Claude Code
- SUMUP_WRITE now returns one ADS error code per sub-request instead of letting a single bad sub-write bubble to the dispatch catch-all as one collapsed batch error. Successful sub-writes still land, and the per-item array is exactly what pyads' own adsSumWriteBytes parses. - The recvfrom guard now logs via logger.exception (matching the dispatch catch) so a genuine socket fault is not swallowed silently, and catches OSError only, since ConnectionResetError is already an OSError subclass. - Add a handler-level regression test for the per-item SUMUP_WRITE codes (the client resolves handles first, so a per-item write failure is not reachable through pyads.Connection). 0x0700 is left as a bare literal to match the file's existing style;
Thanks, all addressed in the latest commit:
|
|
Great work, thanks for your contribution @yannS2016 |
3 participants
testserver: unique notification handles + survive bad input
Three small robustness fixes to the
AdvancedHandlertest server, found while using it to exercise an EPICS ADS driver's bulk-read and notification paths against the full symbol set of a real PLC project.1. Unique notification handles across variables
PLCVariable.register_notificationhanded out colliding handles.notification_countis a class attribute, butself.notification_count += 1rebinds it per-instance, so the first notification registered on every variable returns the same handle (10), the second11, and so on. A client subscribed to more than one variable then cannot tell the notifications apart. Increment the shared class counter so handles are globally unique, as real TwinCAT returns them.2. Survive an abrupt client disconnect
AdsClientConnection.runcalledrecvfrom()with no guard. A client killed mid-request drops the connection abruptly, raisingConnectionResetErrorand crashing the handler thread, which leaves the server unable to serve the next connection. Treat the reset as a disconnect.3. Survive a handler exception
A command handler that raised killed the connection's handler thread, leaving the client's later requests unanswered until they time out.
handle_read_writedoeswrite_data.decode()on the read-write payload, which raisesUnicodeDecodeErrorwhen the payload is not a UTF-8 symbol name (e.g. a binary SUMUP batch), andget_variable_by_nameraises on an unknown symbol. Catch handler exceptions at the dispatch, log them, and return an ADS device error.Testing
tests/test_testserver.pypasses (3/3).read-write carrying a non-UTF-8 name now returns ADS error
0x0700instead of killing the connection thread.