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

Is the --no-sendfile default confusing? #1156

Closed
tilgovi opened this Issue Nov 29, 2015 · 15 comments

Comments

Projects
None yet
4 participants
@tilgovi
Collaborator

tilgovi commented Nov 29, 2015

If you check the usage for gunicorn, the --no-sendfile option says:

  --no-sendfile         Disables the use of ``sendfile()``. [True]

To me, this looks like --no-sendfile defaults to True and that sendfile will by disabled by default.

@tilgovi tilgovi added the Question label Nov 29, 2015

@nMustaki

This comment has been minimized.

Show comment
Hide comment
@nMustaki

nMustaki Nov 29, 2015

I have just noticed this option this afternoon so I can give you my 2cents : the "True" is a bit misleading, but the name of the option itself "no-sendfile" is self-explanatory. I wouldn't expect this to be the default because then it wouldn't have any purpose.

I have just noticed this option this afternoon so I can give you my 2cents : the "True" is a bit misleading, but the name of the option itself "no-sendfile" is self-explanatory. I wouldn't expect this to be the default because then it wouldn't have any purpose.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Nov 29, 2015

Owner

To me, this looks like --no-sendfile defaults to True and that sendfile will by disabled by default.

Me either. This is misleading if you read:
https://github.com/benoitc/gunicorn/blob/master/gunicorn/http/wsgi.py#L349-L350

I guess we should fix it.

Owner

benoitc commented Nov 29, 2015

To me, this looks like --no-sendfile defaults to True and that sendfile will by disabled by default.

Me either. This is misleading if you read:
https://github.com/benoitc/gunicorn/blob/master/gunicorn/http/wsgi.py#L349-L350

I guess we should fix it.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 23, 2015

Owner

@tilgovi what do you think we could do there?

Owner

benoitc commented Dec 23, 2015

@tilgovi what do you think we could do there?

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Dec 24, 2015

Collaborator

https://github.com/benoitc/gunicorn/compare/1156-clarify-sendfile-defaults?expand=1

I'm not sure how much better it is. Usage ends up looking like this:

> gunicorn --help | grep sendfile
  --sendfile [SENDFILE]
                        Enables the use of ``sendfile()``. [True]
  --no-sendfile         Disables the use of ``sendfile()``. [None]

Valid command lines look like these:

  • gunicorn --sendfile -- module:app
  • gunicorn --sendfile false module:app
  • gunicorn --sendfile=false module:app
  • gunicorn --no-sendfile module:app

Default is True. I left the default for --no-sendfile as None so that there was no need to reason about precedence or introduce parser-level defaults to the configuration system.

The other weird thing is the fact that both will show up in the html documentation. Not sure what to do about that.

Feedback welcome.

Collaborator

tilgovi commented Dec 24, 2015

https://github.com/benoitc/gunicorn/compare/1156-clarify-sendfile-defaults?expand=1

I'm not sure how much better it is. Usage ends up looking like this:

> gunicorn --help | grep sendfile
  --sendfile [SENDFILE]
                        Enables the use of ``sendfile()``. [True]
  --no-sendfile         Disables the use of ``sendfile()``. [None]

Valid command lines look like these:

  • gunicorn --sendfile -- module:app
  • gunicorn --sendfile false module:app
  • gunicorn --sendfile=false module:app
  • gunicorn --no-sendfile module:app

Default is True. I left the default for --no-sendfile as None so that there was no need to reason about precedence or introduce parser-level defaults to the configuration system.

The other weird thing is the fact that both will show up in the html documentation. Not sure what to do about that.

Feedback welcome.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Dec 24, 2015

Collaborator

Most confusing is the fact that I current have the name of the --no-sendfile option set to no sendfile. This is weird because it's the only thing that will show up in the documentations with a space instead of an underscore, but I didn't want people to think they could put no_sendfile = True in their gunicorn.conf.py file. Maybe just better documentation would make sure of that, or we could keep the --no-sendfile option out of the documentation to prevent confusion.

Collaborator

tilgovi commented Dec 24, 2015

Most confusing is the fact that I current have the name of the --no-sendfile option set to no sendfile. This is weird because it's the only thing that will show up in the documentations with a space instead of an underscore, but I didn't want people to think they could put no_sendfile = True in their gunicorn.conf.py file. Maybe just better documentation would make sure of that, or we could keep the --no-sendfile option out of the documentation to prevent confusion.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 24, 2015

Owner

why do we have so much options though. IMO sendfile should be either disable or enable. Why not introducing a --disable-sendfile option and deprecate older options?

Owner

benoitc commented Dec 24, 2015

why do we have so much options though. IMO sendfile should be either disable or enable. Why not introducing a --disable-sendfile option and deprecate older options?

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Dec 24, 2015

Collaborator

Fine for me, but this was on the current milestone and I didn't want to
remove a CLI option in a patch release.

On Thu, Dec 24, 2015, 01:09 Benoit Chesneau notifications@github.com
wrote:

why do we have so much options though. IMO sendfile should be either
disable or enable. Why not introducing a --disable-sendfile option and
deprecate older options?


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

Collaborator

tilgovi commented Dec 24, 2015

Fine for me, but this was on the current milestone and I didn't want to
remove a CLI option in a patch release.

On Thu, Dec 24, 2015, 01:09 Benoit Chesneau notifications@github.com
wrote:

why do we have so much options though. IMO sendfile should be either
disable or enable. Why not introducing a --disable-sendfile option and
deprecate older options?


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

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Dec 25, 2015

Collaborator

@tilgovi's patch looks good to me for 19.x.

Collaborator

berkerpeksag commented Dec 25, 2015

@tilgovi's patch looks good to me for 19.x.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Dec 26, 2015

Collaborator

Another option is to introduce parser-level default so --no-sendfile shows as [None] in the usage printout.

What do you think @benoitc?

Collaborator

tilgovi commented Dec 26, 2015

Another option is to introduce parser-level default so --no-sendfile shows as [None] in the usage printout.

What do you think @benoitc?

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Dec 26, 2015

Collaborator

I think I prefer this because it's much simpler, but I don't know if is not False is offensive to anyone ;).

https://github.com/benoitc/gunicorn/compare/1156-clarify-sendfile-defaults-alt?expand=1

Collaborator

tilgovi commented Dec 26, 2015

I think I prefer this because it's much simpler, but I don't know if is not False is offensive to anyone ;).

https://github.com/benoitc/gunicorn/compare/1156-clarify-sendfile-defaults-alt?expand=1

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 26, 2015

Owner

@tilgovi good idea!

What to you think about introducing --sendfile=no command? and deprecate the --no-sendfine ? So we only keep one command line. cc @berkerpeksag

Owner

benoitc commented Dec 26, 2015

@tilgovi good idea!

What to you think about introducing --sendfile=no command? and deprecate the --no-sendfine ? So we only keep one command line. cc @berkerpeksag

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 26, 2015

Owner

@tilgovi i'm fine with the is not false :)

Owner

benoitc commented Dec 26, 2015

@tilgovi i'm fine with the is not false :)

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Dec 26, 2015

Collaborator

Let's merge the second one, the -alt branch I just posted, then.

That is good for a patch release and then we can decide how to handle enable/disable flags and documentation for them for R20.

Collaborator

tilgovi commented Dec 26, 2015

Let's merge the second one, the -alt branch I just posted, then.

That is good for a patch release and then we can decide how to handle enable/disable flags and documentation for them for R20.

@berkerpeksag

This comment has been minimized.

Show comment
Hide comment
@berkerpeksag

berkerpeksag Dec 27, 2015

Collaborator

1156-clarify-sendfile-defaults-alt looks good to me.

Collaborator

berkerpeksag commented Dec 27, 2015

1156-clarify-sendfile-defaults-alt looks good to me.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 27, 2015

Owner

+1
On Sun, 27 Dec 2015 at 09:20, Berker Peksag notifications@github.com
wrote:

1156-clarify-sendfile-defaults-alt looks good to me.


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

Owner

benoitc commented Dec 27, 2015

+1
On Sun, 27 Dec 2015 at 09:20, Berker Peksag notifications@github.com
wrote:

1156-clarify-sendfile-defaults-alt looks good to me.


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

@tilgovi tilgovi closed this in d8b6f0a Dec 27, 2015

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

Clarify --no-sendfile default
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment