Wrong HTTPupload size member semantics in ESP8266WebServer

[everslick]

IMHO the semantics of HTTPupload.currentSize and HTTPupload.totalSize are wrong or at least highly misleading.

now we get the size of the last chunk of data in currentSize and the number of bytes received so far in totalSize. But currentSize as it is, is pretty useless and we really want the know how big the file is going to be once complete (for correct progress calculation and for checking filesystem requirements).

i suggest to make currentSize what totalSize is now and put the Content-Length header arg in totalSize.

does this sound reasonable?

[devyte]

@igrr I think this would silently break who knows what in very subtle ways. Maybe add accumSize?

[igrr]

How about expectedSize for the value from Content-Length header?

…

On Fri, Nov 3, 2017, 23:43 Develo ***@***.***> wrote: @igrr <https://github.com/igrr> I think this would silently break who knows what in very subtle ways. Maybe add accumSize? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3787 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEJcei6pgyy3coEXIkLduKvyto-GRC45ks5syzSXgaJpZM4QQ0j9> .

[everslick]

typedef struct {
  HTTPUploadStatus status;
  String  filename;
  String  name;
  String  type;
  size_t  totalSize;    // file size
  size_t  currentSize;  // size of data currently in buf
  uint8_t buf[HTTP_UPLOAD_BUFLEN];
} HTTPUpload;

the comment for total size should be changed to 'total number of bytes received until now' or something like that. as for the name for the new member. why not simply add size_t fileSize; // expected number of bytes from Content-Length ?

[vdeconinck]

+1 for expectedSize without breaking current behaviour
(and better comment / documentation for each field)

[aderusha]

Is there any way to collect the total size of the expected transfer today through this library?

[RickeyWard]

Is there any way to collect the total size of the expected transfer today through this library?

Yes, if you set your server to store the "Content-Length" header, you can then fetch it later.

  const char * headerkeys[] = {"Content-Length"} ;
  size_t headerkeyssize = sizeof(headerkeys) / sizeof(char*);
  //ask server to track this headers
  server.collectHeaders(headerkeys, headerkeyssize);
  server.begin();

if you don't, the server object will not hold onto this value.

[RickeyWard]

I push for adding content-length to the upload struct. It's going to be off by around 200 bytes. But it would be very handy to have. The content length is already parsed into an int in the Parsing.cpp of Esp8266WebServer, So adding it to the struct and setting the value there is a trivial change. I'd be happy to make a pull request if the community is for it. Would be nice to count all the bytes read in until the post body starts, so we would have the exact size of the payload, but I don't know if the overhead of doing that is worth 200ish bytes of lost precision.

Thoughts?

[aderusha]

In my use case I require a precise value, so off by 200 or even 1 byte is problematic. I wound up using javascript to send the filesize as a REST call upon selecting the file before attempting upload, which is an ugly hack that'd best be avoided if we could otherwise somehow get the actual filesize through the server object.

[RickeyWard]

I've added a pull request that provides this simplified but still useful version.
Pull Request

[RickeyWard]

In my use case I require a precise value, so off by 200 or even 1 byte is problematic. I wound up using javascript to send the filesize as a REST call upon selecting the file before attempting upload, which is an ugly hack that'd best be avoided if we could otherwise somehow get the actual filesize through the server object.

Unfortunately there's only 2 ways to get it, Compute it, or send it. Sending it puts more work on the client and makes it much harder to bake into the library, but computing it provides extra overhead for the ESP. It would take a bit of rewriting to make sure that the offset if property kept track of and it would still only work in the case that the browser sends a content length, which isn't explicitly required by the spec.

I just made this change because it's nice to have SOMETHING to expect, even if it's not perfect. I'd like to be able to immediately abort if they user tries to upload a huge file to freeze and crash my device.

[everslick]

I see that a backwards compatible solution was merged. -> closing