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

feat: add unstable background method to subscriber/queryable/matching listeners #1106

Merged
merged 9 commits into from
Jun 10, 2024

Conversation

wyfo
Copy link
Contributor

@wyfo wyfo commented Jun 10, 2024

Following #1012, but completely reworked (no more builder method), hence a new PR.

Based on #1104, should wait to have it merge before merging this PR.

wyfo added 2 commits June 10, 2024 11:10
This flag was just used to prevent double close/undeclaration, i.e. object being drop after `close`/`undeclare`.
Using `ManuallyDrop` instead in `close`/`undeclare` solve this issue, and save some space in structs
(often one word counting the padding, which is not negligible).
wyfo added 3 commits June 10, 2024 13:40
…ng listeners

The only real change of this PR is to undeclare objects at session closing/publisher undeclaration.
Calling `background` is then only a `mem::forget`.
@wyfo wyfo changed the title Background feat: add unstable background method to subscriber/queryable/matching listeners Jun 10, 2024
@wyfo
Copy link
Contributor Author

wyfo commented Jun 10, 2024

@OlivierHecart you might be interested

@wyfo
Copy link
Contributor Author

wyfo commented Jun 10, 2024

@Mallets I've integrated our discussion (see the comments) and fixed my stupid mistakes. I've also renamed all the alive flags into close_on_drop/undeclare_on_drop, because explicit is better, and because this spelling is better with background feature.

#[inline]
pub fn set_allowed_destination(&mut self, destination: Locality) {
Copy link
Contributor

@milyin milyin Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happened with set_allowed_destination? Is it's removal planned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discussed about it with @OlivierHecart weeks ago, this thing is currently broken (the modification is not transmitted to the PublisherState stored in session, only the creation destination matter).
There was maybe another reason, but I forgot. @OlivierHecart do you remember?

@milyin milyin merged commit 7d5d1d4 into eclipse-zenoh:dev/1.0.0 Jun 10, 2024
9 of 11 checks passed
@@ -350,6 +358,13 @@ impl<'a> Publisher<'a> {
pub fn undeclare(self) -> impl Resolve<ZResult<()>> + 'a {
Undeclarable::undeclare_inner(self, ())
}

fn undeclare_matching_listeners(&self) -> ZResult<()> {
for id in zlock!(self.matching_listeners).drain() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code would be safer w.r.t. mutex deadlock if:

  1. it takes the lock
  2. it drains the iterator in a supporting vec
  3. it releases the lock
  4. it undeclares the matching listeners

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants