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

Use plain socket objects instead of wrapper classes #3127

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tilgovi
Copy link
Collaborator

@tilgovi tilgovi commented Dec 28, 2023

Refactor socket creation to remove the socket wrapper classes so that these objects have less surprising behavior when used in worker hooks, worker classes, and custom applications.

Close #3013.

Refactor socket creation to remove the socket wrapper classes so that
these objects have less surprising behavior when used in worker hooks,
worker classes, and custom applications.

Close #3013.
if i < 5:
msg = "connection to {addr} failed: {error}"
log.debug(msg.format(addr=str(addr), error=str(e)))
log.error("Retrying in 1 second.")
Copy link
Contributor

Choose a reason for hiding this comment

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

The error-severity message should have whatever we know about the error, the debug-severity should convey the non-news "Just in case you have not guessed already, absolutely fantastically nothing will happen in the next second" .
It was like that before, but should be fixed as part of this patch, because this one has some potential to make the difference matter.

Copy link
Owner

Choose a reason for hiding this comment

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

@pajod what's your idea there?

Copy link
Contributor

@pajod pajod Mar 9, 2024

Choose a reason for hiding this comment

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

Swap severity. Make the error appear in error logs. Demote sleep interval to debug logs.

for i in range(1,6):
    try:
        ...
    except socket.error as e:
        ...
        error_msg = "connection attempt %d/5 to %s failed: %s"
        log.error(error_msg, i, addr, e)
        if i < 5:
            log.debug("Retrying in 1 second.")
            time.sleep(1)

Submitted separately in #3191

@jsprieto10
Copy link

is there anything else missing for this?

@tilgovi
Copy link
Collaborator Author

tilgovi commented Mar 5, 2024

@benoitc polite bump, if you have the time

@benoitc
Copy link
Owner

benoitc commented Mar 6, 2024

@tilgovi is this ready for review or do yui think to any other addition/changes already?

@tnusser
Copy link

tnusser commented Apr 17, 2024

@tilgovi What is the status of your PR?

# first initialization of gunicorn
old_umask = os.umask(conf.umask)
try:
for addr in [bind for bind in conf.address if not isinstance(bind, int)]:
Copy link
Owner

Choose a reason for hiding this comment

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

can you make this part more readable. I would create the bind list in a separate line there.

Copy link

Choose a reason for hiding this comment

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

Does my PR work for you?

Copy link
Owner

@benoitc benoitc left a comment

Choose a reason for hiding this comment

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

@tilgovi can you handle the change there. Otherwise I think it's OK for merge.

tnusser added a commit to tnusser/gunicorn that referenced this pull request Apr 17, 2024
Implementing suggestion by reviewer benoitc#3127 (comment) to make code more readable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad file descriptor problem still exists as of today with uvicorn workers
5 participants