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

Correctly handle unavailable .fileno() support for io.BytesIO instances #1174

Closed
wbolster opened this Issue Dec 31, 2015 · 16 comments

Comments

Projects
None yet
5 participants
@wbolster

wbolster commented Dec 31, 2015

The changes introduced in 98c9e3b#diff-6607b435d6ee868f251dbdd1a6d0edecL357 are wrong and lead to crashes for io.BytesIO instances which do have a .fileno() method but raise another exception instead:

>>> import io
>>> io.BytesIO().fileno()
Traceback (most recent call last):
  File "<ipython-input-3-7062c09d0c52>", line 1, in <module>
    io.BytesIO().fileno()
UnsupportedOperation: fileno

This exception was already caught properly in the earlier code, a few lines below.

wbolster referenced this issue Dec 31, 2015

Catch sendfile failure from no file descriptor
If the filelike response object has no `fileno` attribute, then skip
trying to use sendfile rather than failing with an error.

Close #1160

@wbolster wbolster changed the title from properly handle unavailable fileno for bytesio to Correctly handle unavailable `fileno()` support for `io.BytesIO` instances Dec 31, 2015

@wbolster wbolster changed the title from Correctly handle unavailable `fileno()` support for `io.BytesIO` instances to Correctly handle unavailable .fileno() support for io.BytesIO instances Dec 31, 2015

@wbolster

This comment has been minimized.

Show comment
Hide comment
@wbolster

wbolster Dec 31, 2015

I guess this is a critical bug since it breaks a lot of cases like flask.send_file() (and other frameworks with similar features) with a io.BytesIO() instance, which is a rather common pattern.

wbolster commented Dec 31, 2015

I guess this is a critical bug since it breaks a lot of cases like flask.send_file() (and other frameworks with similar features) with a io.BytesIO() instance, which is a rather common pattern.

benoitc added a commit that referenced this issue Dec 31, 2015

reuse util.is_fileobject
is_fileobject usgae was removed due to the use of the `tell` method check.
This change remove this check wich allows us to completely test if
fileno() is usable. Also it handle most of the exceptions around created by
breaking changes across Python versions. Hopefully we are good now.

fix #1174
@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 31, 2015

Owner

@wbolster thanks for the ticket. Can you test the change above?

@tilgovi what do you think about it?

Owner

benoitc commented Dec 31, 2015

@wbolster thanks for the ticket. Can you test the change above?

@tilgovi what do you think about it?

@wbolster

This comment has been minimized.

Show comment
Hide comment
@wbolster

wbolster Dec 31, 2015

fwiw, BytesIO is specifically intended to be a file like object (according to the vague python definition of that term), so having a function is_fileobject() return False violates the principle of least surprise. :)

I guess we were both looking at this at the same time, and I just posted #1176 with an alternative...

wbolster commented Dec 31, 2015

fwiw, BytesIO is specifically intended to be a file like object (according to the vague python definition of that term), so having a function is_fileobject() return False violates the principle of least surprise. :)

I guess we were both looking at this at the same time, and I just posted #1176 with an alternative...

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Dec 31, 2015

Owner

@wbolster Thanks for the patch!

The solutions are similar but I quite like the idea of having a function apart for it, so it may be later unit-tested eventually.

File objects were better defined in old versions (and Gunicorn starts to get old) [1] . I see now that it has been reduced. Although this tests is done there to test if the file-like object return a usable file descriptior so we can use sendfile(2) .So indeed we should change the name of the function to make it more descriptive. Thoughts?

[1] https://docs.python.org/2.5.2/lib/bltin-file-objects.html
[2] https://docs.python.org/3/glossary.html#term-file-object

Owner

benoitc commented Dec 31, 2015

@wbolster Thanks for the patch!

The solutions are similar but I quite like the idea of having a function apart for it, so it may be later unit-tested eventually.

File objects were better defined in old versions (and Gunicorn starts to get old) [1] . I see now that it has been reduced. Although this tests is done there to test if the file-like object return a usable file descriptior so we can use sendfile(2) .So indeed we should change the name of the function to make it more descriptive. Thoughts?

[1] https://docs.python.org/2.5.2/lib/bltin-file-objects.html
[2] https://docs.python.org/3/glossary.html#term-file-object

@benoitc benoitc added the bug :( label Dec 31, 2015

@wbolster

This comment has been minimized.

Show comment
Hide comment
@wbolster

wbolster Dec 31, 2015

is_usable_for_sendfile(), or has_fileno(). or just obtain_fileno() in true eafp style but that requires a well defined exception.

wbolster commented Dec 31, 2015

is_usable_for_sendfile(), or has_fileno(). or just obtain_fileno() in true eafp style but that requires a well defined exception.

@DCudmore

This comment has been minimized.

Show comment
Hide comment
@DCudmore

DCudmore Jan 2, 2016

Hi,

I'm not sure if this is exactly related, but I'm having a similar issue with the following Traceback:

Traceback (most recent call last):
File "/home/dale/.local/lib/python2.7/site-packages/gunicorn/workers/sync.py", line 130, in handle
self.handle_request(listener, req, client, addr)
File "/home/dale/.local/lib/python2.7/site-packages/gunicorn/workers/sync.py", line 174, in handle_request
resp.write_file(respiter)
File "/home/dale/.local/lib/python2.7/site-packages/gunicorn/http/wsgi.py", line 398, in write_file
if not self.sendfile(respiter):
File "/home/dale/.local/lib/python2.7/site-packages/gunicorn/http/wsgi.py", line 357, in sendfile
fileno = respiter.filelike.fileno()

Here is a git copy if it helps: https://github.com/DCuddies/simplecontract

I'm using BytesIO in order to store a docx object, and then let the users download that docx object.

I'm at a loss at this point as to how to fix the issue, any help would be appreciated.

DCudmore commented Jan 2, 2016

Hi,

I'm not sure if this is exactly related, but I'm having a similar issue with the following Traceback:

Traceback (most recent call last):
File "/home/dale/.local/lib/python2.7/site-packages/gunicorn/workers/sync.py", line 130, in handle
self.handle_request(listener, req, client, addr)
File "/home/dale/.local/lib/python2.7/site-packages/gunicorn/workers/sync.py", line 174, in handle_request
resp.write_file(respiter)
File "/home/dale/.local/lib/python2.7/site-packages/gunicorn/http/wsgi.py", line 398, in write_file
if not self.sendfile(respiter):
File "/home/dale/.local/lib/python2.7/site-packages/gunicorn/http/wsgi.py", line 357, in sendfile
fileno = respiter.filelike.fileno()

Here is a git copy if it helps: https://github.com/DCuddies/simplecontract

I'm using BytesIO in order to store a docx object, and then let the users download that docx object.

I'm at a loss at this point as to how to fix the issue, any help would be appreciated.

@benoitc benoitc closed this in #1175 Jan 2, 2016

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Jan 2, 2016

Owner

@DCuddies sorry have been side tracked. Patch is now is master. If everything goes fine, i will make a release over the week-end.

Owner

benoitc commented Jan 2, 2016

@DCuddies sorry have been side tracked. Patch is now is master. If everything goes fine, i will make a release over the week-end.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Jan 2, 2016

Owner

@wbolster merged #1175 . Patch makes sure that in case the fileno method doesn't exist or is unavailable we fallback to the iteration loop.

Owner

benoitc commented Jan 2, 2016

@wbolster merged #1175 . Patch makes sure that in case the fileno method doesn't exist or is unavailable we fallback to the iteration loop.

@DCudmore

This comment has been minimized.

Show comment
Hide comment
@DCudmore

DCudmore Jan 2, 2016

@benoitc That's great, thanks.

DCudmore commented Jan 2, 2016

@benoitc That's great, thanks.

@tilgovi

This comment has been minimized.

Show comment
Hide comment
@tilgovi

tilgovi Jan 2, 2016

Collaborator

Sorry I have been offline. Thanks for the support @benoitc. I will be more proactive about getting good feedback and testing, even for these one line fixes, going forward.

Collaborator

tilgovi commented Jan 2, 2016

Sorry I have been offline. Thanks for the support @benoitc. I will be more proactive about getting good feedback and testing, even for these one line fixes, going forward.

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Jan 3, 2016

Owner

@tilgovi np :) thanks for all the work you already do anyway :)

Owner

benoitc commented Jan 3, 2016

@tilgovi np :) thanks for all the work you already do anyway :)

@wbolster

This comment has been minimized.

Show comment
Hide comment
@wbolster

wbolster Jan 4, 2016

awesome. any chance of cutting a new release? currently pip install gunicorn results in broken deployments in many environments. (gunicorn is often treated differently from normal requirements.txt since it's not really a dependency but a "runner", so this may even slip past version pinning best practices for many people...)

wbolster commented Jan 4, 2016

awesome. any chance of cutting a new release? currently pip install gunicorn results in broken deployments in many environments. (gunicorn is often treated differently from normal requirements.txt since it's not really a dependency but a "runner", so this may even slip past version pinning best practices for many people...)

@benoitc

This comment has been minimized.

Show comment
Hide comment
@benoitc

benoitc Jan 4, 2016

Owner

19.4.4 has just been released. enjoy! cc @wbolster @DCuddies

Owner

benoitc commented Jan 4, 2016

19.4.4 has just been released. enjoy! cc @wbolster @DCuddies

@tuco86

This comment has been minimized.

Show comment
Hide comment
@tuco86

tuco86 Jan 4, 2016

Contributor

So i'm at 19.4.4 and:

Traceback (most recent call last):
  File ".env/lib/python2.7/site-packages/gunicorn/workers/async.py", line 52, in handle
    self.handle_request(listener_name, req, client, addr)
  File ".env/lib/python2.7/site-packages/gunicorn/workers/ggevent.py", line 163, in handle_request
    super(GeventWorker, self).handle_request(*args)
  File ".env/lib/python2.7/site-packages/gunicorn/workers/async.py", line 110, in handle_request
    resp.write_file(respiter)
  File ".env/lib/python2.7/site-packages/gunicorn/http/wsgi.py", line 396, in write_file
    if not self.sendfile(respiter):
  File ".env/lib/python2.7/site-packages/gunicorn/http/wsgi.py", line 360, in sendfile
    offset = os.lseek(fileno, 0, os.SEEK_CUR)
NameError: global name 'fileno' is not defined

this doesn't seem right either, does it?

Contributor

tuco86 commented Jan 4, 2016

So i'm at 19.4.4 and:

Traceback (most recent call last):
  File ".env/lib/python2.7/site-packages/gunicorn/workers/async.py", line 52, in handle
    self.handle_request(listener_name, req, client, addr)
  File ".env/lib/python2.7/site-packages/gunicorn/workers/ggevent.py", line 163, in handle_request
    super(GeventWorker, self).handle_request(*args)
  File ".env/lib/python2.7/site-packages/gunicorn/workers/async.py", line 110, in handle_request
    resp.write_file(respiter)
  File ".env/lib/python2.7/site-packages/gunicorn/http/wsgi.py", line 396, in write_file
    if not self.sendfile(respiter):
  File ".env/lib/python2.7/site-packages/gunicorn/http/wsgi.py", line 360, in sendfile
    offset = os.lseek(fileno, 0, os.SEEK_CUR)
NameError: global name 'fileno' is not defined

this doesn't seem right either, does it?

@tuco86

This comment has been minimized.

Show comment
Hide comment
@tuco86

tuco86 Jan 4, 2016

Contributor

see #1178

Contributor

tuco86 commented Jan 4, 2016

see #1178

@DCudmore

This comment has been minimized.

Show comment
Hide comment
@DCudmore

DCudmore Jan 8, 2016

The update fixed it, thanks!

DCudmore commented Jan 8, 2016

The update fixed it, thanks!

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

reuse util.is_fileobject
is_fileobject usgae was removed due to the use of the `tell` method check.
This change remove this check wich allows us to completely test if
fileno() is usable. Also it handle most of the exceptions around created by
breaking changes across Python versions. Hopefully we are good now.

fix #1174
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment