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

Only require &self for Connection::close #54

Closed
Cryptex-github opened this issue Feb 12, 2023 · 7 comments
Closed

Only require &self for Connection::close #54

Cryptex-github opened this issue Feb 12, 2023 · 7 comments
Labels
question Further information is requested

Comments

@Cryptex-github
Copy link

I am currently trying to implement a connection pool for amqprs using deadpool, which only provides a &mut Connection for the recycle method, so I would greatly appreciate if close only requires &self, or provide a way to manually set is_open.

@gftea
Copy link
Owner

gftea commented Feb 12, 2023

Hi, manually set is_open will alllow user to mutate the internal state of connection which break the the contract of open and close, I would perfer not to go this way.

I am not familar with deadpool, can you elaborate further why &self is necessary? current API will avoid user to use the connection unexpectedly after close the connection.
Can the new type pattern works for you? Usually we can implement new type in order to suite the contract required by other libraries

@Cryptex-github
Copy link
Author

Hi, manually set is_open will alllow user to mutate the internal state of connection which break the the contract of open and close, I would perfer not to go this way.

I am not familar with deadpool, can you elaborate further why &self is necessary? current API will avoid user to use the connection unexpectedly after close the connection. Can the new type pattern works for you? Usually we can implement new type in order to suite the contract required by other libraries

So deadpool is a connection pool, it allows user to implement their own pool with whatever library if they implement the Manager trait for their own Manager, the trait signature is as below:

#[async_trait]
impl managed::Manager for Manager {
    type Type = Connection;
    type Error = amqprs::error::Error;

    async fn create(&self) -> Result<Self::Type, Self::Error> {
        Ok(Connection::open(&self.con_args).await?)
    }

    async fn recycle(&self, conn: &mut Self::Type) -> RecycleResult<Self::Error> {
        todo!()
    }

Now the only method we are concerned here is recycle, which only provides a &mut Self::Type, while I am not entirely sure why, but is likely due to how deadpool holds each objects.

Can the new type pattern works for you? Usually we can implement new type in order to suite the contract required by other libraries

I am not entirely sure about what that means, can you please further elaborate?

@gftea
Copy link
Owner

gftea commented Feb 13, 2023

As I understand, you should not call close in recycle because it must be alive and be reused in a pool.

you can read about new type pattern, see explanation

@Cryptex-github
Copy link
Author

As I understand, you should not call close in recycle because it must be alive and be reused in a pool.

you can read about new type pattern, see explanation

Whoops, yes you are right I misunderstood what it means by recycle (should have looked at examples), based on the assumption that is_open will get automatically set to false when the connection closed, even if no callback is registered?

@gftea
Copy link
Owner

gftea commented Feb 14, 2023

is_open will get automatically set to false when the connection closed, even if no callback is registered?

That’s right

@gftea
Copy link
Owner

gftea commented Feb 15, 2023

if no more issue with it, I would close it in the near future

@gftea gftea added the question Further information is requested label Feb 15, 2023
@Cryptex-github
Copy link
Author

if no more issue with it, I would close it in the near future

Yes there is no more issue related to this topic, feel free to close it if you want to.

@gftea gftea closed this as completed Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants