Fix upload speed inflation from HTTP errors + document server body size limit#820
Fix upload speed inflation from HTTP errors + document server body size limit#820EmanuelPeixoto wants to merge 2 commits into
Conversation
- speedtest_worker.js: xhr.upload.onload now checks xhr.status before restarting the upload stream. Non-2xx responses (e.g. HTTP 413 from undersized client_max_body_size) are treated as failures instead of inflating upload speed measurements. - doc.md: added explicit nginx/Apache/IIS configuration examples for client_max_body_size / LimitRequestBody to prevent HTTP 413 errors on the 20MB upload test blobs.
There was a problem hiding this comment.
Code review
Changes requested — the HTTP-status check is attached to the wrong XHR lifecycle event.
xhr.upload.onload fires when the request body has been uploaded, which does not guarantee that response headers (and therefore xhr.status) are available. Valid requests can still have status 0 at that point and will be treated as failures/restarted.
Please perform the status check and stream restart from xhr.onload instead. Keep xhr.upload.onprogress for byte accounting, and handle transport failures with xhr.onerror.
Also, the documentation currently mentions IIS but does not include the promised web.config example or clarify that maxAllowedContentLength is expressed in bytes.
| totLoaded += loadDiff; | ||
| prevLoaded = event.loaded; | ||
| }.bind(this); | ||
| xhr[i].upload.onload = function() { |
There was a problem hiding this comment.
upload.onload marks completion of sending the request body, not completion of the HTTP transaction. The server response/status may not be available yet, so xhr.status can be 0 even for a successful upload. Put this status-dependent logic in xhr.onload (and use xhr.onerror for network errors) so a completed response is what determines whether to restart or fail the stream.
There was a problem hiding this comment.
Thanks for the update. The status check is now attached to xhr.onload, but the previous xhr.upload.onload handler still starts the replacement request first.
That means the later xhr.onload callback reads xhr[i] after it may have been replaced by the new request, rather than examining the XHR that received the response. A 413 can therefore still be associated with the wrong request (often status 0) and abort/delete the replacement stream.
Please remove the upload.onload restart and make xhr.onload own the complete lifecycle: check the captured x.status, restart only after a 2xx response, and retain the configured retry/fail handling for non-2xx responses. Using x rather than xhr[i] in these callbacks also avoids this slot-replacement race.
- speedtest_worker.js: moved HTTP status check from xhr.upload.onload (body sent, response may not be available) to xhr.onload (full HTTP transaction complete, xhr.status is reliable). - doc.md: added full IIS web.config example with maxAllowedContentLength.
2 participants
What
speedtest_worker.js:xhr.upload.onloadnow checks HTTP status before restarting the upload stream. Non-2xx responses (e.g. HTTP 413 from undersizedclient_max_body_size) aretreated as failures instead of inflating upload speed measurements.
doc.md: added explicit nginx/Apache/IIS config examples forclient_max_body_size/LimitRequestBodyto prevent HTTP 413 errors on 20 MB upload blobs.Why
Server returning 413 (body too large) caused
onloadto fire instantly with the worker interpreting it as a successful upload, reporting impossible speeds (7-10 Gbps on gigabit LAN).Fixes both the symptom (code) and the root cause (docs).
Closes #819