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

Try to reuse already created connection and channel while emitting from external processes #935

Closed
wants to merge 2 commits into from

Conversation

theDigitalGuy
Copy link

@theDigitalGuy theDigitalGuy commented Jun 16, 2022

to avoid rabbitmq starting to block connections.

Problem description: theDigitalGuy@4ff70fb#commitcomment-76278910

@miguelgrinberg
Copy link
Owner

miguelgrinberg commented Jun 16, 2022

Hi, thanks for this. There's two problems:

  1. Your solution is not robust enough. It should handle random disconnects and retries. See the Redis implementations to have an idea of what is expected.
  2. The Kombu and aiopika implementations are more or less the same. If one is improved, the other should be improved in the same way.

Do you think you can address these?

@sillyfrog
Copy link

Regarding (1), the RobustConnection object returned by connect_robust actively reconnects on errors/disconnects etc. I've done extensive testing in single node and clustered configurations to confirm it's very robust.

While there is no connection to the server an error is raised, but as soon as connectivity is restored, things start working again as you would hope, (probably) negating the need for this to be re-implemented in python-socketio.

Comment on lines +58 to +60
if (self.aiopika_connection is None or self.aiopika_connection.is_closed is True):
return await aio_pika.connect_robust(self.url)
return self.aiopika_connection

Choose a reason for hiding this comment

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

This should have a lock around it as if there are several calls in close succession, it'll create several new connections. See #1142 for an example of this in use.

@miguelgrinberg
Copy link
Owner

Fixed by cd7f781.

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

Successfully merging this pull request may close these issues.

3 participants