Repository navigation
fix(client): keep failed monitored items out of the recreated subscription - #2028
Merged
Merged
Conversation
…ption create_monitored_items registers items client-side before the server call so that notifications arriving ahead of the response are not lost. When the whole service call failed, that registration was never undone: per-item failures were removed, a service-level failure was not. With auto_reconnect the supervisor's recreate() then carried those half-registered items into the new subscription, so a subscribe_data_change() that raised BadSubscriptionIdInvalid ended up delivering notifications anyway (FreeOpcUa#2020). The registry mutations now clean up when the service call raises, and a call that started against one incarnation of the subscription and returns against another is told so rather than reporting success for work the server no longer knows about. Ordering these with a lock was the obvious approach and the wrong one: a call holding it while stuck on a dropped connection blocks the very reconnect that would free it, which deadlocks the client — worse than the bug being fixed. A generation counter gives the same guarantee and never waits. recreate() also carries over only items the server confirmed, since adopting one still in flight would make its call raise while its node was quietly being watched.
oroulet
reviewed
Sep 1, 2026
oroulet
reviewed
Sep 1, 2026
oroulet
reviewed
Sep 1, 2026
Contributor
Author
|
Done. The comments are gone, the wrapper methods too (all five, they were left over from the lock attempt), and the counter is now |
oroulet
reviewed
Sep 12, 2026
| return | ||
| saved_items = list(self._monitored_items.values()) | ||
| self._recreate_count += 1 | ||
| saved_items: list[SubscriptionItemData] = [ |
Member
There was a problem hiding this comment.
precise these are montored items. I ha to read that several times to understand
oroulet
reviewed
Sep 12, 2026
| mids.append(result.MonitoredItemId) | ||
| return mids | ||
|
|
||
| def _forget(self, requests: list[ua.MonitoredItemCreateRequest], recreate_count: int) -> None: |
Member
|
some more comments but I think it looks ok |
Contributor
Author
|
Done — renamed to saved_monitored_items and _forget_monitored_items. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2020.
create_monitored_itemsregisters items client-side before the server call, so that notifications arriving ahead of the response are not lost. When the whole service call failed that registration was never undone — per-item failures were removed, a service-level failure was not — andrecreate()then carried those half-registered items into the new subscription. Asubscribe_data_change()that raisedBadSubscriptionIdInvalidended up delivering notifications anyway.Three things changed:
recreate()carries over only items the server confirmed. Adopting one still in flight would make its call raise while its node was quietly being watched — the same bug through another door.On the approach. Ordering these with a lock was my first attempt and it was wrong: a call holding the lock while stuck on a dropped connection blocks the very reconnect that would free it. That deadlocks the client, which is worse than the bug being fixed — a test pins it. A generation counter gives the same guarantee and never waits.
Measured with the reporter's kill-and-restart sequence:
subscribe_data_change()now genuinely succeeds instead of raising while working. The suite is unchanged,ruff checkandmypyare clean.