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

pool: auto-close subscription after Duration of no new events #693

Merged
merged 1 commit into from
Dec 27, 2024

Conversation

yukibtc
Copy link
Member

@yukibtc yukibtc commented Dec 26, 2024

No description provided.

@yukibtc yukibtc force-pushed the no-events-timeout branch 3 times, most recently from 94167e3 to 35d4c6f Compare December 26, 2024 10:23
@yukibtc
Copy link
Member Author

yukibtc commented Dec 26, 2024

@dluvian what do you think? Should the SubscribeAutoCloseOptions::relative_timeout be renamed to something else?

@dluvian
Copy link
Contributor

dluvian commented Dec 26, 2024

@dluvian what do you think? Should the SubscribeAutoCloseOptions::relative_timeout be renamed to something else?

I like idle_timeout or inactivity_timeout. When I see relative_timeout I would have to check the docs to know what it is meant to do.

pub fn timeout(&self, timeout: Option<Duration>) -> Self {
let mut builder = self.clone();
builder.inner = builder.inner.timeout(timeout);
builder
}

/// Automatically close subscription if don't receive any more notifications/events within the duration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Automatically close subscription if don't receive any more notifications/events within the duration.
/// Automatically close subscription if you don't receive any more notifications/events within the duration.
Suggested change
/// Automatically close subscription if don't receive any more notifications/events within the duration.
/// Automatically close subscription if no notifications/events are received within the duration.

@@ -171,6 +171,12 @@ impl JsSubscribeAutoCloseOptions {
pub fn timeout(self, timeout: Option<JsDuration>) -> Self {
self.inner.timeout(timeout.map(|t| *t)).into()
}

/// Automatically close subscription if don't receive any more notifications/events within the duration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Automatically close subscription if don't receive any more notifications/events within the duration.
/// Automatically close subscription if you don't receive any more notifications/events within the duration.
Suggested change
/// Automatically close subscription if don't receive any more notifications/events within the duration.
/// Automatically close subscription if no notifications/events are received within the duration.

pub fn timeout(mut self, timeout: Option<Duration>) -> Self {
self.timeout = timeout;
self
}

/// Automatically close subscription if don't receive any more notifications/events within the [`Duration`].
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Automatically close subscription if don't receive any more notifications/events within the [`Duration`].
/// Automatically close subscription if you don't receive any more notifications/events within the [`Duration`].
Suggested change
/// Automatically close subscription if don't receive any more notifications/events within the [`Duration`].
/// Automatically close subscription if no notifications/events are received within the [`Duration`].

Add `SubscribeAutoCloseOptions::relative_timeout`

Closes #691

Co-authored-by: dluvian <dluvian@proton.me>
Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
@yukibtc yukibtc merged commit 60a10b1 into master Dec 27, 2024
7 checks passed
@yukibtc yukibtc deleted the no-events-timeout branch December 27, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants