Skip to content
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

Handle when the server terminates the upload early. #37

Merged
merged 1 commit into from
Mar 16, 2017

Conversation

christoph-neumann
Copy link
Contributor

I can't figure out how to get GitHub to re-open pull request #36 with the new changes, so I'm opening another pull request.

@christoph-neumann
Copy link
Contributor Author

All it does is adjust the byte count down based on how much data node buffered. Using the nginx setup I mentioned in PR #36. It seems to produce much more realistic stats:

   { download: 44.156,  // Internet server
     upload: 793.425,   // nginx on localhost
     originalDownload: 3868719,
     originalUpload: 45978368 }

nginx starts returning HTTP 413 at 2MB. I ran a few tests. These are each 5 second runs. They each use nginx on localhost for uploads (via docker in a VM on a Mac). The "original" is the code before this patch. The "with fix" is the code with this patch. The "uploads <= 1M" uses the original code, but avoids upload sizes that trigger HTTP 413.

setup run 1 run 2 run 3 run 4
original 3742.4 3871.4 3200.4 2179.2
with fix 800.8 739.4 725.0 773.1
uploads <= 1M 628.0 578.6 582.9 596.8

You can see the numbers are still a bit inflated, but it's still an improvement. (Buffered bytes are not transferred bytes, after all.) The ideal would be to respond to a short upload by backing down on the size or measuring actual transfer. But if you could at least merge this fix, it would make this case smoother until someone has time to refactor.

@ddsol
Copy link
Owner

ddsol commented Mar 16, 2017

As soon as the node http module accepts the buffer we give it, this assumes we sent it. This isn't the case.

413 is special in that it is sent before the request arrives completely. As a matter of fact, as soon as the headers are in, the server knows the request is too big and will respond with a 413 right away.

We can't assume we actually sent any data. Instead, a 413 should cause a size cap to be set that we adhere to in subsequent requests. Also, the 413'd request should be completely discarded as it has no significance for speed measurements.

IOW, this PR does improve things, but doesn't fix them the right way, and certainly not accurately. I'd fix it myself but I'm a busy bee. I'm hoping you'll do it instead!

@ddsol ddsol merged commit 5c11159 into ddsol:master Mar 16, 2017
@christoph-neumann
Copy link
Contributor Author

Thanks for merging that stopgap. I appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants