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

Avoid 'Connection refused' when using gthread with keep-alive #1699

Merged
merged 1 commit into from Mar 1, 2018

Conversation

Projects
None yet
3 participants
@ddzialak
Copy link
Contributor

ddzialak commented Feb 6, 2018

Fixes #1698

@benoitc

This comment has been minimized.

Copy link
Owner

benoitc commented Feb 6, 2018

Thanks for the patch! Looking :)

@ddzialak

This comment has been minimized.

Copy link
Contributor Author

ddzialak commented Feb 8, 2018

any update?

@ddzialak

This comment has been minimized.

Copy link
Contributor Author

ddzialak commented Feb 8, 2018

I've added two commits to make added attribute corresponding to it's name, these commits don't affect solution for connection refused problem. Now is_active should be True if it's processed and should not be present on self._keep and False if processing is done and should be present on self._keep. Please squash these commits if everything is ok.

@tilgovi
Copy link
Collaborator

tilgovi left a comment

Does any of this get simpler to reason about or handle if the murder_keepalived method held the lock for the whole body of the loop? With the lock being taken in both branches of the if statement on line 168, I wonder whether it's worth the overhead of locking twice and whether holding the lock throughout might remove the race condition without introducing this new variable.

@ddzialak

This comment has been minimized.

Copy link
Contributor Author

ddzialak commented Feb 11, 2018

@tilgovi That was my first try and after that received connection refused again but most probably I've done something wrong with testing..

@ddzialak ddzialak force-pushed the ddzialak:master branch from 535558a to c705441 Feb 12, 2018

@ddzialak ddzialak force-pushed the ddzialak:master branch from c705441 to cdf82cd Feb 12, 2018

@ddzialak

This comment has been minimized.

Copy link
Contributor Author

ddzialak commented Feb 13, 2018

Now it's clean why moving whole murder_keepalived under lock doesn't solved that issue. Other important condition is that bug may happen only with Python2 because __cmp__ in python2 is used to override == operator while function __lt__ in Python3 doesn't.

@ddzialak

This comment has been minimized.

Copy link
Contributor Author

ddzialak commented Feb 19, 2018

any update?

@benoitc benoitc merged commit 9c8695b into benoitc:master Mar 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benoitc

This comment has been minimized.

Copy link
Owner

benoitc commented Mar 1, 2018

Sorry for the delay. looks good, merging it now :) Thanks !

@ddzialak

This comment has been minimized.

Copy link
Contributor Author

ddzialak commented Mar 1, 2018

@ddzialak

This comment has been minimized.

Copy link
Contributor Author

ddzialak commented Mar 7, 2018

@benoitc What is the estimated time when this change is pushed to PyPI?

fofanov pushed a commit to fofanov/gunicorn that referenced this pull request Mar 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.