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

fix to decrement open connections count on connect exception #1

Merged
merged 3 commits into from
Apr 7, 2017

Conversation

V0idmain
Copy link

@V0idmain V0idmain commented Apr 6, 2017

Fixes the issue where the pool connection count in incremented even if their is an error while creating the connection. Based on an earlier PR that was never merged.

@benjsto
Copy link

benjsto commented Apr 7, 2017

This code looks good to me but I think we'll obv want to test it in stage with an existing service (like flexpro_api) to make sure it a) solves the problem we're trying to solve and b) doesn't create any other weird behavior.

@V0idmain
Copy link
Author

V0idmain commented Apr 7, 2017

@benjsto agreed. Can you pull it in to flexpro, push it and we can test?

@benjsto
Copy link

benjsto commented Apr 7, 2017

I can do that. Won't get to that until Monday most likely..


# Wait to other connection is released.
fut = Future()
self._waitings.append(fut)
return fut

def _on_connect(self, fut):
logging.info('error')

Choose a reason for hiding this comment

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

Looks like this should be inside the if block below?

Copy link
Author

Choose a reason for hiding this comment

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

thanks @dangnvang will just remove it.

@dangnvang
Copy link

@benjsto: I'll take a shot at a PR for flexpro

@benjsto
Copy link

benjsto commented Apr 7, 2017

Thanks Dang!

@V0idmain V0idmain merged commit 00f4739 into master Apr 7, 2017
@V0idmain V0idmain deleted the fix_error_on_connect branch April 7, 2017 20:22
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.

3 participants