-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
base: master
Are you sure you want to change the base?
Conversation
e4a2331
to
bb6c318
Compare
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.
bb6c318
to
a24ff07
Compare
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.") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
is there anything else missing for this? |
@benoitc polite bump, if you have the time |
@tilgovi is this ready for review or do yui think to any other addition/changes already? |
@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)]: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
Implementing suggestion by reviewer benoitc#3127 (comment) to make code more readable.
Should the monkey patching in I was able to run my WIP request after reloading test against your PR after fixing just the following variable access: - sockets.append(socket.socket(s.FAMILY, socket.SOCK_STREAM,
- fileno=s.sock.fileno()))
+ sockets.append(socket.socket(s.family, socket.SOCK_STREAM,
+ fileno=s.fileno())) |
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.