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

Updated gunicorn/pidfile.py #1042

Merged
merged 3 commits into from Jul 9, 2015

Conversation

Projects
None yet
5 participants
@brijeshb42
Contributor

brijeshb42 commented Jun 4, 2015

Added a try block to convert the contents of the pidfile.py if it can be converted to integer or not. If not, then wpid is assigned 0 and the validate function returns as usual. If the pidfile has a valid int, the function continues to kill the process as before.

Updated gunicorn/pidfile.py
Added a try block to convert the contents of the `pidfile.py` if it can be converted to integer or not. If not, then `wpid` is assigned 0 and the `validate` function returns as usual. If the pidfile has a valid int, the function continues to kill the process as before.
@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Jun 4, 2015

Owner

+1

Owner

benoitc commented Jun 4, 2015

+1

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Jun 4, 2015

Collaborator

LGTM. Just curious: How did you noticed this? Did you see a traceback or just read the code?

Collaborator

berkerpeksag commented Jun 4, 2015

LGTM. Just curious: How did you noticed this? Did you see a traceback or just read the code?

@brijeshb42

This comment has been minimized.

Show comment
Hide comment
@brijeshb42

brijeshb42 Jun 4, 2015

Contributor

I was trying to run a Flask app using gunicorn but the pidfile that I was providing was already created but it had a single empty line in it. So it was not starting and giving invalid literal for int() with base 10 ''. Then through the stacktrace, I was able to go to that particular line. So I added an extra check.

Contributor

brijeshb42 commented Jun 4, 2015

I was trying to run a Flask app using gunicorn but the pidfile that I was providing was already created but it had a single empty line in it. So it was not starting and giving invalid literal for int() with base 10 ''. Then through the stacktrace, I was able to go to that particular line. So I added an extra check.

@emamirazavi

This comment has been minimized.

Show comment
Hide comment
@emamirazavi

emamirazavi Jun 4, 2015

Why don't i see this problem?!

On Thu, Jun 4, 2015 at 1:23 PM, Brijesh Bittu notifications@github.com
wrote:

I was trying to run a Flask app using gunicorn but the pidfile that I was
providing was already created but its had a single empty line in it. So it
was not starting and giving invalid literal for int() with base 10 ''.
Then through the stacktrace, I was able to go to that particular line. So I
added an extra check.


Reply to this email directly or view it on GitHub
#1042 (comment).

emamirazavi commented Jun 4, 2015

Why don't i see this problem?!

On Thu, Jun 4, 2015 at 1:23 PM, Brijesh Bittu notifications@github.com
wrote:

I was trying to run a Flask app using gunicorn but the pidfile that I was
providing was already created but its had a single empty line in it. So it
was not starting and giving invalid literal for int() with base 10 ''.
Then through the stacktrace, I was able to go to that particular line. So I
added an extra check.


Reply to this email directly or view it on GitHub
#1042 (comment).

@brijeshb42

This comment has been minimized.

Show comment
Hide comment
@brijeshb42

brijeshb42 Jun 4, 2015

Contributor

I was using gunicorn:0.19.3 .
I guess this problem won't appear if the pidfile you are supplying does not exist or contains a valid integer or exists but is completely empty.
Mine had a single empty line.

Contributor

brijeshb42 commented Jun 4, 2015

I was using gunicorn:0.19.3 .
I guess this problem won't appear if the pidfile you are supplying does not exist or contains a valid integer or exists but is completely empty.
Mine had a single empty line.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Jun 6, 2015

Owner

hrm f.read() or 0 should fix the empty case normally. The valid integer case is still valid. Which version of Python are you using?

Owner

benoitc commented Jun 6, 2015

hrm f.read() or 0 should fix the empty case normally. The valid integer case is still valid. Which version of Python are you using?

@brijeshb42

This comment has been minimized.

Show comment
Hide comment
@brijeshb42

brijeshb42 Jun 9, 2015

Contributor

I am using sys.version_info(major=2, minor=7, micro=6, releaselevel='final', serial=0)

Contributor

brijeshb42 commented Jun 9, 2015

I am using sys.version_info(major=2, minor=7, micro=6, releaselevel='final', serial=0)

@tilgovi

View changes

Show outdated Hide outdated gunicorn/pidfile.py
Update pidfile.py
Return directly in the `except` instead of checking again for zero (as suggested by @benoitc and @tilgovi)
@brijeshb42

This comment has been minimized.

Show comment
Hide comment
@brijeshb42

brijeshb42 Jul 9, 2015

Contributor

@benoitc Updated the code.

Contributor

brijeshb42 commented Jul 9, 2015

@benoitc Updated the code.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Jul 9, 2015

Collaborator

I think the or 0 just has to be removed. The intention is that if read returns the empty string that int raises ValueError and that is how we can skip the subsequent if.

Collaborator

tilgovi commented Jul 9, 2015

I think the or 0 just has to be removed. The intention is that if read returns the empty string that int raises ValueError and that is how we can skip the subsequent if.

Removed `or 0`
Removed `or 0` to raise `ValueError` for empty pidfile or pidfile with invalid contents.
@brijeshb42

This comment has been minimized.

Show comment
Hide comment
@brijeshb42

brijeshb42 Jul 9, 2015

Contributor

@tilgovi added your suggestion.

Contributor

brijeshb42 commented Jul 9, 2015

@tilgovi added your suggestion.

tilgovi added a commit that referenced this pull request Jul 9, 2015

@tilgovi tilgovi merged commit d971eb4 into benoitc:master Jul 9, 2015

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Jul 9, 2015

Collaborator

Thanks!

Collaborator

tilgovi commented Jul 9, 2015

Thanks!

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

Merge pull request #1042 from brijeshb42/master
Updated gunicorn/pidfile.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment