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

Client does not disconnect from relays upon Drop #137

Closed
benthecarman opened this issue Jul 16, 2023 · 7 comments
Closed

Client does not disconnect from relays upon Drop #137

benthecarman opened this issue Jul 16, 2023 · 7 comments

Comments

@benthecarman
Copy link
Contributor

When a Client is dropped it still persists the connections to the relays, this is an easy footgun that can cause resource exhaustion.

The user needs to remember to call disconnect() otherwise.

@yukibtc yukibtc reopened this Jul 17, 2023
@yukibtc
Copy link
Member

yukibtc commented Jul 17, 2023

I tried to implement Drop for RelayPool but this cause many issues in a multi-threads env (like https://github.com/coinstr/coinstr) where the Client is cloned and dropped many times.

For now the only way to completely shutdown the client is to exit from the app (the threads will drop) or by calling the shutdown method.

@yukibtc
Copy link
Member

yukibtc commented Jul 17, 2023

An alternative to shutdown is the stop method: it allow to re-start a previously constructed Client (anyway it must be called manually) by calling the start method.

@benthecarman
Copy link
Contributor Author

Just something to kill the connections on drop would be nice, have run into the problem multiple times on accident

@yukibtc
Copy link
Member

yukibtc commented Jul 17, 2023

Maybe you can wrap the Client:

pub struct CustomClient {
    inner: Client
}

impl Drop for CustomClient {
    fn drop(&mut self) {
        let client = self.clone();
        async_utility::thread::spawn(async move {
            client.shutdown().await.unwrap();
        });
    }
}

I can't implement the Drop because if a user do this

let client = self.clone();
thread::spawn(async move {
    // use client and then drop
});

the client shutdown everywhere.

@yukibtc
Copy link
Member

yukibtc commented Jul 17, 2023

Otherwise, I'll try to add a new filed in Options, like shutdown_on_drop: bool (by default will be disabled).
If you'll initialize the Client with that option enabled, the Client will shutdown on drop.
What do you think?

@benthecarman
Copy link
Contributor Author

That sounds good

@yukibtc
Copy link
Member

yukibtc commented Jul 17, 2023

Done

So, init the Client in this way:

// ...
let opts = Options::new().shutdown_on_drop(true);
let client = Client::with_opts(&keys, opts);
// ...

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

No branches or pull requests

2 participants