Skip to content

Commit

Permalink
BugFix #636 Pause and resume (In a quick succession) gets you lot of …
Browse files Browse the repository at this point in the history
…exceptions and an infinite loop (#637)
  • Loading branch information
varunsharma27 authored and Sergi Almacellas Abellana committed Mar 19, 2019
1 parent a627547 commit f293071
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 5 deletions.
21 changes: 16 additions & 5 deletions papaparse.js
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,7 @@ License: MIT
this._handle = null;
this._finished = false;
this._completed = false;
this._halted = false;
this._input = null;
this._baseIndex = 0;
this._partialLine = '';
Expand All @@ -497,15 +498,18 @@ License: MIT
chunk = modifiedChunk;
}
this.isFirstChunk = false;
this._halted = false;

// Rejoin the line we likely just split in two by chunking the file
var aggregate = this._partialLine + chunk;
this._partialLine = '';

var results = this._handle.parse(aggregate, this._baseIndex, !this._finished);

if (this._handle.paused() || this._handle.aborted())
if (this._handle.paused() || this._handle.aborted()) {
this._halted = true;
return;
}

var lastIndex = results.meta.cursor;

Expand All @@ -531,8 +535,10 @@ License: MIT
else if (isFunction(this._config.chunk) && !isFakeChunk)
{
this._config.chunk(results, this._handle);
if (this._handle.paused() || this._handle.aborted())
if (this._handle.paused() || this._handle.aborted()) {
this._halted = true;
return;
}
results = undefined;
this._completeResults = undefined;
}
Expand Down Expand Up @@ -992,7 +998,6 @@ License: MIT
// One goal is to minimize the use of regular expressions...
var FLOAT = /^\s*-?(\d*\.?\d+|\d+\.?\d*)(e[-+]?\d+)?\s*$/i;
var ISO_DATE = /(\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d\.\d+([+-][0-2]\d:[0-5]\d|Z))|(\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d:[0-5]\d([+-][0-2]\d:[0-5]\d|Z))|(\d{4}-[01]\d-[0-3]\dT[0-2]\d:[0-5]\d([+-][0-2]\d:[0-5]\d|Z))/;

var self = this;
var _stepCounter = 0; // Number of times step was called (number of rows parsed)
var _rowCounter = 0; // Number of rows that have been parsed so far
Expand Down Expand Up @@ -1089,8 +1094,14 @@ License: MIT

this.resume = function()
{
_paused = false;
self.streamer.parseChunk(_input, true);
if(self.streamer._halted) {
_paused = false;
self.streamer.parseChunk(_input, true);
} else {
// Bugfix: #636 In case the processing hasn't halted yet
// wait for it to halt in order to resume
setTimeout(this.resume, 3);

This comment has been minimized.

Copy link
@jczhong84

jczhong84 Mar 2, 2020

@varunsharma27 is this should be self.resume instead of this.resume? found that this is null here.

This comment has been minimized.

Copy link
@varunsharma27

varunsharma27 Mar 3, 2020

Author Contributor

Looking at the code, it makes sense. this -> self should fix the undefined issue. Is there any open issue for it?

This comment has been minimized.

Copy link
@pokoli

pokoli Mar 3, 2020

Collaborator

could you please create a PR fixing this issue?

This comment has been minimized.

Copy link
@varunsharma27

varunsharma27 Mar 3, 2020

Author Contributor

Done, tested locally: #769

}
};

this.aborted = function()
Expand Down
36 changes: 36 additions & 0 deletions tests/node-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,42 @@ describe('PapaParse', function() {
assertLongSampleParsedCorrectly(Papa.parse(longSampleRawCsv));
});

it('Pause and resume works (Regression Test for Bug #636)', function(done) {
this.timeout(30000);
var mod200Rows = [
["Etiam a dolor vitae est vestibulum","84","DEF"],
["Etiam a dolor vitae est vestibulum","84","DEF"],
["Lorem ipsum dolor sit","42","ABC"],
["Etiam a dolor vitae est vestibulum","84","DEF"],
["Etiam a dolor vitae est vestibulum","84"],
["Lorem ipsum dolor sit","42","ABC"],
["Etiam a dolor vitae est vestibulum","84","DEF"],
["Etiam a dolor vitae est vestibulum","84","DEF"],
["Lorem ipsum dolor sit","42","ABC"],
["Lorem ipsum dolor sit","42"]
];
var stepped = 0;
var dataRows = [];
Papa.parse(fs.createReadStream(__dirname + '/verylong-sample.csv'), {
step: function(results, parser) {
stepped++;
if (results)
{
parser.pause();
parser.resume();
if (results.data && stepped % 200 === 0) {
dataRows.push(results.data);
}
}
},
complete: function() {
assert.strictEqual(2001, stepped);
assert.deepEqual(mod200Rows, dataRows);
done();
}
});
});

it('asynchronously parsed CSV should be correctly parsed', function(done) {
Papa.parse(longSampleRawCsv, {
complete: function(parsedCsv) {
Expand Down
46 changes: 46 additions & 0 deletions tests/test-cases.js
Original file line number Diff line number Diff line change
Expand Up @@ -1780,6 +1780,49 @@ describe('Unparse Tests', function() {


var CUSTOM_TESTS = [
{
description: "Pause and resume works (Regression Test for Bug #636)",
disabled: !XHR_ENABLED,
timeout: 30000,
expected: [2001, [
["Etiam a dolor vitae est vestibulum","84","DEF"],
["Etiam a dolor vitae est vestibulum","84","DEF"],
["Lorem ipsum dolor sit","42","ABC"],
["Etiam a dolor vitae est vestibulum","84","DEF"],
["Etiam a dolor vitae est vestibulum","84"],
["Lorem ipsum dolor sit","42","ABC"],
["Etiam a dolor vitae est vestibulum","84","DEF"],
["Etiam a dolor vitae est vestibulum","84","DEF"],
["Lorem ipsum dolor sit","42","ABC"],
["Lorem ipsum dolor sit","42"]
], 0],
run: function(callback) {
var stepped = 0;
var dataRows = [];
var errorCount = 0;
var output = [];
Papa.parse(BASE_PATH + "verylong-sample.csv", {
download: true,
step: function(results, parser) {
stepped++;
if (results)
{
parser.pause();
parser.resume();
if (results.data && stepped % 200 === 0) {
dataRows.push(results.data);
}
}
},
complete: function() {
output.push(stepped);
output.push(dataRows);
output.push(errorCount);
callback(output);
}
});
}
},
{
description: "Complete is called with all results if neither step nor chunk is defined",
expected: [['A', 'b', 'c'], ['d', 'E', 'f'], ['G', 'h', 'i']],
Expand Down Expand Up @@ -2263,6 +2306,9 @@ var CUSTOM_TESTS = [
describe('Custom Tests', function() {
function generateTest(test) {
(test.disabled ? it.skip : it)(test.description, function(done) {
if(test.timeout) {
this.timeout(test.timeout);
}
test.run(function(actual) {
assert.deepEqual(JSON.stringify(actual), JSON.stringify(test.expected));
done();
Expand Down

0 comments on commit f293071

Please sign in to comment.