Skip to content

Commit

Permalink
Clarify --no-sendfile default
Browse files Browse the repository at this point in the history
The --no-sendfile option had a confusing entry in the usage message.
Even though sendfile is enabled by default, the --no-sendfile flag
showed a true value as the default, which could be interpreted to
mean that by default sendfile support is disabled.

This change makes the default "None", meaning sendfile is not
disabled, which is hopefully slightly more clear.

Close #1156
  • Loading branch information
tilgovi committed Dec 27, 2015
1 parent 2ac41c4 commit d8b6f0a
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 3 deletions.
6 changes: 4 additions & 2 deletions gunicorn/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -821,13 +821,14 @@ class PreloadApp(Setting):
restarting workers.
"""


class Sendfile(Setting):
name = "sendfile"
section = "Server Mechanics"
cli = ["--no-sendfile"]
validator = validate_bool
action = "store_false"
default = True
action = "store_const"
const = False

This comment has been minimized.

Copy link
@benoitc

benoitc Dec 28, 2015

Owner

@tilgovi this change breaks the tests since no default is given. Fixed by #1168, let me know what you think about that change.

desc = """\
Disables the use of ``sendfile()``.
Expand All @@ -837,6 +838,7 @@ class Sendfile(Setting):
disabling.
"""


class Chdir(Setting):
name = "chdir"
section = "Server Mechanics"
Expand Down
2 changes: 1 addition & 1 deletion gunicorn/http/wsgi.py
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ def write(self, arg):
util.write(self.sock, arg, self.chunked)

def can_sendfile(self):
return self.cfg.sendfile and sendfile is not None
return self.cfg.sendfile is not False and sendfile is not None

def sendfile(self, respiter):
if self.cfg.is_ssl or not self.can_sendfile():
Expand Down

4 comments on commit d8b6f0a

@kevgliss
Copy link

Choose a reason for hiding this comment

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

Weirdly the change from store_false to store_const is freaking out argparse. I take guinicorn's options and combine them with my own options. Still digging to see I can find the cause.

@tilgovi
Copy link
Collaborator Author

@tilgovi tilgovi commented on d8b6f0a Jan 5, 2016

Choose a reason for hiding this comment

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

@kevgliss let me know if I can help.

@benoitc
Copy link
Owner

@benoitc benoitc commented on d8b6f0a Jan 6, 2016

Choose a reason for hiding this comment

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

@kevgliss can you describe what happens?

@kevgliss
Copy link

Choose a reason for hiding this comment

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

@benoitc I have described what I have figured out so far here:
Netflix/lemur#204

For me this throws an error deep down in argparse.py still verifying if its a side effect of flask-script. I'm not positive it's a problem with gunicorn, but is an interesting side effect of this change.

Please sign in to comment.