Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove channel upon undeclaration of Subscriber and Queryable #147

Merged
merged 4 commits into from
Mar 4, 2025

Conversation

Charles-Schleich
Copy link
Member

@Charles-Schleich Charles-Schleich commented Mar 4, 2025

Fixes #139

Note: Due to the timing difference in between issuing an UndeclareSubscriber from the front-end to the plugin, this change may cause the TS bindings to warn that a "Subscrption UUID not in map" Depending on the number of outstanding messages that still need to be flushed from the plugin before the undeclare is executed in the plugin.

The state of a Subscriber lives on the Plugin, we have changed the way the TS client and the plugin handle undeclaring Subscribers.
The flow is as follows:

  1. TS client sends Undeclare Message to Plugin with Subscruber UUID
  2. Plugin aborts Future that Subscriber lives on (effectively undeclaring the Subscriber as per Drop impl in Zenoh), it is gaurenteed no more messages will be sent to the client once this future has been aborted.
  3. Plugin explicitly sends back to the client an undeclare message with the SubscriberUUID to the TS client, which then removes the SubscriberChannel from the corresponding map in the RemoteSession.

This should resolve the uncaught exception that occurs when another node floods this client with many samples, and allow a new subscriber to be declared on the same session.

Copy link

github-actions bot commented Mar 4, 2025

PR missing one of the required labels: {'bug', 'breaking-change', 'documentation', 'enhancement', 'new feature', 'dependencies', 'internal'}

@Charles-Schleich Charles-Schleich added the internal Changes not included in the changelog label Mar 4, 2025
@Charles-Schleich Charles-Schleich requested a review from milyin March 4, 2025 15:02
@milyin
Copy link
Contributor

milyin commented Mar 4, 2025

Fixes #139

Note: Due to the timing difference in between issuing an UndeclareSubscriber from the front-end to the plugin, this change may cause the TS bindings to warn that a "Subscrption UUID not in map" Depending on the number of outstanding messages that still need to be flushed from the plugin before the undeclare is executed in the plugin.

Not clear how this solution can provoke "Subscrption UUID not in map" warning. As far as I see nothing happens until the moment when "Undeclare" reply is received by zenoh-ts from plugin

@Charles-Schleich
Copy link
Member Author

@milyin I have updated the comment on the Issue, to clarify the new behaviour.

@Charles-Schleich Charles-Schleich requested a review from milyin March 4, 2025 16:48
@milyin milyin enabled auto-merge (squash) March 4, 2025 16:49
@milyin milyin merged commit 0b4b692 into eclipse-zenoh:main Mar 4, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Changes not included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undeclare on a subscriber can break subscribers (impossible to receive Samples anymore)
2 participants