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

Pubsub/Monitor support #5

Merged
merged 6 commits into from
Sep 15, 2023

Conversation

wilgaboury
Copy link
Contributor

No description provided.

@wilgaboury
Copy link
Contributor Author

wilgaboury commented Sep 15, 2023

Don't merge this PR just yet, I realized that monitors cannot be turned back into a connection, so it's drop function is actually causing an async infinite loop.

I don' think there's a good reason to support Monitor in that case because once turned into one the connection cannot be returned to the pool.

@genusistimelord
Copy link
Member

yeah that is why I said Monitor will need to be treated as if the connection never really existed to the pool. So a Get_monitor where it gets a connection without creating anything for the pool probably would be better.

@wilgaboury
Copy link
Contributor Author

I added a factory function to the pool so that users can access the underlying client without having to go through the pool, which should cover that use case.

@genusistimelord
Copy link
Member

So was that able to fix the monitor issue?

@wilgaboury
Copy link
Contributor Author

Yup should be okay to merge as long as you think the changes look good.

Copy link
Member

@genusistimelord genusistimelord left a comment

Choose a reason for hiding this comment

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

everything looks good. Thank you!

@genusistimelord genusistimelord merged commit e6a1601 into AscendingCreations:main Sep 15, 2023
1 check passed
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