Skip to content

Commit 307a1c6

Browse files
gijzelaerrclaude
andcommitted
Fix multi-read optimizer: cache mutation, dead code, and usability issues
- Fix cache mutation bug: deep-copy cached ReadPackets before assigning buffers so repeated calls don't corrupt cached state - Remove dead _map_area_int method - Replace per-call set comprehension with module-level _VALID_AREA_VALUES frozenset for Area validation - Fix O(n^2) byte concatenation in build_multi_read_request using list and b"".join() - Clear optimizer cache on disconnect - Add use_optimizer attribute (default True) to allow opting out Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent fa56347 commit 307a1c6

File tree

2 files changed

+18
-13
lines changed

2 files changed

+18
-13
lines changed

snap7/client.py

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
Drop-in replacement for the ctypes-based client with native Python implementation.
55
"""
66

7+
import copy
78
import logging
89
import random
910
import struct
@@ -41,6 +42,8 @@
4142
CDataArrayType,
4243
)
4344

45+
_VALID_AREA_VALUES: frozenset[int] = frozenset(a.value for a in Area)
46+
4447
logger = logging.getLogger(__name__)
4548

4649

@@ -136,6 +139,7 @@ def __init__(
136139
# Multi-read optimizer state
137140
self._opt_plan: Optional[_OptimizationPlan] = None
138141
self.multi_read_max_gap: int = 5
142+
self.use_optimizer: bool = True
139143

140144
# Async operation state
141145
self._async_pending = False
@@ -430,6 +434,7 @@ def disconnect(self) -> int:
430434

431435
self.connected = False
432436
self._is_alive = False
437+
self._opt_plan = None
433438
logger.info(f"Disconnected from {self.host}:{self.port}")
434439
return 0
435440

@@ -721,8 +726,8 @@ def read_multi_vars(self, items: Union[List[dict[str, Any]], "Array[S7DataItem]"
721726
# Dict list path -- use optimizer for 2+ items
722727
dict_items = cast(List[dict[str, Any]], items)
723728

724-
if len(dict_items) <= 1:
725-
# Single item: no optimization needed
729+
if len(dict_items) <= 1 or not self.use_optimizer:
730+
# Single item or optimizer disabled: no optimization needed
726731
results: list[bytearray] = []
727732
for dict_item in dict_items:
728733
area = dict_item["area"]
@@ -773,15 +778,18 @@ def _read_multi_vars_optimized(self, dict_items: List[dict[str, Any]]) -> Tuple[
773778
packets = packetize(blocks, self.pdu_length)
774779
self._opt_plan = _OptimizationPlan(cache_key, packets, read_items)
775780

781+
# Deep-copy blocks from cached packets so we don't mutate cached state
782+
working_packets = copy.deepcopy(packets)
783+
776784
# Execute each packet
777-
for packet in packets:
785+
for packet in working_packets:
778786
block_specs = [(blk.area, blk.db_number, blk.start_offset, blk.byte_length) for blk in packet.blocks]
779787

780788
if len(block_specs) == 1:
781789
# Single block: use regular read to avoid multi-read overhead
782790
blk = packet.blocks[0]
783791
data = self.read_area(
784-
Area(blk.area) if blk.area in {a.value for a in Area} else Area.DB,
792+
Area(blk.area) if blk.area in _VALID_AREA_VALUES else Area.DB,
785793
blk.db_number,
786794
blk.start_offset,
787795
blk.byte_length,
@@ -796,13 +804,9 @@ def _read_multi_vars_optimized(self, dict_items: List[dict[str, Any]]) -> Tuple[
796804
blk.buffer = buf
797805

798806
# Extract per-item results in original order
799-
results = extract_results(packets, len(dict_items))
807+
results = extract_results(working_packets, len(dict_items))
800808
return (0, results)
801809

802-
def _map_area_int(self, area_int: int) -> S7Area:
803-
"""Map integer area value to S7Area enum."""
804-
return S7Area(area_int)
805-
806810
def write_multi_vars(self, items: Union[List[dict[str, Any]], List[S7DataItem]]) -> int:
807811
"""
808812
Write multiple variables in a single request.

snap7/s7protocol.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -188,13 +188,14 @@ def build_multi_read_request(self, items: List[Tuple[int, int, int, int]]) -> by
188188
item_count = len(items)
189189

190190
# Build N * 12-byte address specifications
191-
addr_specs = b""
191+
addr_spec_parts: list[bytes] = []
192192
for area_code, db_number, start_offset, byte_length in items:
193-
addr_spec = S7DataTypes.encode_address(S7Area(area_code), db_number, start_offset, S7WordLen.BYTE, byte_length)
194-
addr_specs += addr_spec
193+
addr_spec_parts.append(
194+
S7DataTypes.encode_address(S7Area(area_code), db_number, start_offset, S7WordLen.BYTE, byte_length)
195+
)
195196

196197
# Parameter: function_code(1) + item_count(1) + N * address_spec(12)
197-
param_data = struct.pack(">BB", S7Function.READ_AREA, item_count) + addr_specs
198+
param_data = struct.pack(">BB", S7Function.READ_AREA, item_count) + b"".join(addr_spec_parts)
198199
param_len = len(param_data)
199200

200201
# S7 Header (12 bytes)

0 commit comments

Comments
 (0)