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

Cannot close new connections opened by Client after closing once #48

Closed
KiithNabaal opened this issue May 19, 2021 · 2 comments
Closed

Comments

@KiithNabaal
Copy link
Contributor

I create a Client singleton object and open/close a single connection on that Client only whenever I need to consume from RabbitMQ. I can open and close a connection just fine the first time, but when I open a connection a second time on that same Client object, that connection doesn't close.

This is because of the check on line 237 in client_impl.dart:

if (_clientClosed != null) { return _clientClosed.future; }

_clientClosed is set to null when the Client object is first created, but after the connection itself is closed, _clientClosed isn't set back to null. This means the check above always returns a Future, rather than proceeding to the code to close the connection.

It could be that the library doesn't intend for Clients to be re-used like this, but my assumption was it's okay to re-use a Client. Right now as a work-around I just create a new Client object each time I need to establish a new connection to RabbitMQ.

I could submit a fix for this, but wanted to ensure that you all agree this is a bug and how to write unit tests for this.

Thanks!

@achilleasa
Copy link
Owner

Thanks for reporting this! It is definitely a bug; the client implementation needs a _clientClosed = null statement in https://github.com/achilleasa/dart_amqp/blob/master/lib/src/client/impl/client_impl.dart#L255 to prevent the second close attempt from returning the Future as you suggest.

Feel free to submit a PR. I believe a simple connect/disconnect N times kind of test would suffice as a unit test for the fix.

@achilleasa
Copy link
Owner

Fixed via #49.

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