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

Fix Range Header processing logic for NetworkStreamer #709

Merged
merged 4 commits into from
Sep 17, 2019

Conversation

t83714
Copy link
Contributor

@t83714 t83714 commented Sep 17, 2019

Not all HTTP servers implement the correct behaviour of processing Range header for partial download.

Some HTTP server may ignore the range-end parameter and return the file content from the start index to the end of the file.

e.g.:
Request:

Range: bytes=0-2097151
Sec-Fetch-Mode: cors
User-Agent: Mozilla/5.0 

Response:

accept-ranges: bytes
Content-Length: 8256106
Content-Range: bytes 0-8256106/8256106
server: nginx/1.13.12
status: 206, 206 Partial Content

As we increase the _start index by chunkSize config option at each round of chunk processing (instead of increasing the _start by the actual size of the response), papaparse may incorrect re-download & parse unnecessary chunk:

PapaParse/papaparse.js

Lines 673 to 688 in 94d7bf9

this._start += this._config.chunkSize;
};
this._chunkLoaded = function()
{
if (xhr.readyState !== 4)
return;
if (xhr.status < 200 || xhr.status >= 400)
{
this._chunkError();
return;
}
this._finished = !this._config.chunkSize || this._start > getFileSize(xhr);
this.parseChunk(xhr.responseText);

This PR allows Papaparse to handle this situation and avoid downloading file content for many times.

…Range-end paramenter of the http Range header and return more content than expected
Copy link
Collaborator

@pokoli pokoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your proposal.

Please do not modify the minified file it will be automatically modified on our release process.

I've added a comment on the PR, once fixed I will be happy to merge the fix.

@t83714
Copy link
Contributor Author

t83714 commented Sep 17, 2019

@pokoli Could you please have another look?
I have removed the empty lines and changes to the minified file.

@pokoli pokoli merged commit 9b54b11 into mholt:master Sep 17, 2019
@pokoli
Copy link
Collaborator

pokoli commented Sep 17, 2019

@t83714 I've merged it. Thanks!

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