Opened 6 years ago

Closed 4 years ago

#13901 closed Bug (wontfix)

File.chunks fails to read the entire file for some file-like objects

Reported by: Ian Kelly Owned by: nobody
Component: File uploads/storage Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The File.chunks method gets the size of the file and then decrements a counter by the chunk size for each read until it reaches 0. The problem is that the read method is not guaranteed to return the full amount requested, and so the amount read may be less than the chunk size, resulting in the counter being decremented erroneously. I ran into this bug while using a file-like object that wraps a java.io.InputStream object, whose read method returns as soon as data is available.

This patch is one possible solution, although it still has the problem that it will result in a busy-loop if the read method is entirely non-blocking. It also potentially yields non-terminal chunks smaller than the chunk size, which may not be desirable.

See also #9632, which may affect the implementation here if it is accepted.

Attachments (2)

file_chunks.diff (518 bytes) - added by Ian Kelly 6 years ago.
file_chunks_2.diff (513 bytes) - added by Ian Kelly 6 years ago.
Corrected patch

Download all attachments as: .zip

Change History (11)

Changed 6 years ago by Ian Kelly

Attachment: file_chunks.diff added

comment:1 Changed 6 years ago by Alex Gaynor

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

This patch doesn't look right, it should be len(chunk , not chunk_size. Also I'm pretty sure this is a dupe.

Changed 6 years ago by Ian Kelly

Attachment: file_chunks_2.diff added

Corrected patch

comment:2 Changed 6 years ago by Ian Kelly

You're at least right about the first part. That's what I get for once again writing a small patch without testing it. I've uploaded a corrected patch.

comment:3 Changed 6 years ago by Karen Tracey

#13756 is very similar (may be the dupe Alex was thinking of)...same issue but different bit of code.

comment:4 Changed 6 years ago by Alex Gaynor

That is indeed what I was thinking of.

comment:5 Changed 6 years ago by dmoisset

Needs tests: set
Triage Stage: UnreviewedAccepted

Patch is ok, but needs a regression test.

(The issue on non-blocking read happens in the existing codebase, and should be treated as another ticket if considered a problem... although using a File with a non blocking read source looks like a bad idea anyway)

comment:6 Changed 6 years ago by Graham King

Severity: Normal
Type: Bug

comment:7 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:8 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:9 Changed 4 years ago by Claude Paroz

Resolution: wontfix
Status: newclosed

The implementation of the chunks() method has changed in r17871, to not depend on size anymore. Hence rendering this ticket obsolete.

Note: See TracTickets for help on using tickets.
Back to Top