Skip to content

Commit

Permalink
Merge pull request #687 from DaNish808/more-memory-#677
Browse files Browse the repository at this point in the history
Prevent out-of-memory error
  • Loading branch information
nellh authored Jan 18, 2019
2 parents 39cec46 + 306db5a commit e5b3816
Show file tree
Hide file tree
Showing 15 changed files with 218 additions and 179 deletions.
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"jshint": "^2.9.6",
"minimatch": "3.0.4",
"nifti-js": "^1.0.1",
"p-limit": "^2.1.0",
"pako": "^1.0.6",
"path": "^0.12.7",
"pluralize": "^7.0.0",
Expand Down
5 changes: 0 additions & 5 deletions tests/bids.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,7 @@ describe('BIDS example datasets ', function() {
'tests/data/bids-examples-' + global.test_version + '/' + path + '/',
options,
function(issues) {
var errors = issues.errors
var warnings = issues.warnings
assert.deepEqual(
errors.filter(issue => issue.key !== 'EMPTY_FILE'),
[],
)
var session_flag = false
for (var warning in warnings) {
if (warnings[warning]['code'] === '38') {
Expand Down
1 change: 1 addition & 0 deletions tests/utils/files.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ describe('validateMisc', () => {
})
it('returns issues for empty files (0kb)', done => {
const files = groupFileTypes(filelist, {})
utils.collectSummary(filelist, {})

validateMisc(files.misc).then(issues => {
assert.ok(issues.length > 0)
Expand Down
25 changes: 10 additions & 15 deletions utils/files/validateMisc.js
Original file line number Diff line number Diff line change
@@ -1,25 +1,20 @@
const readFile = require('./readFile')
const Issue = require('../issues/issue')

function createIssueForEmpty(file) {
const size = typeof window !== 'undefined' ? file.size : file.stats.size
return size <= 0 && new Issue({ code: 99, file: file })
}
function clearNonIssues(x) {
return x instanceof Issue
}

/**
* validateMisc
*
* takes a list of files and returns an issue for each file
*/
module.exports = function validateMisc(miscFiles) {
const issuePromises = miscFiles.reduce(
(issues, file) => [
...issues,
readFile(file, false, null).catch(err => {
if (err instanceof Issue) return err
throw err
}),
],
[],
)
return (
Promise.all(issuePromises)
// remove non-issues
.then(res => res.filter(o => o instanceof Issue))
return Promise.resolve(
miscFiles.map(createIssueForEmpty).filter(clearNonIssues),
)
}
2 changes: 2 additions & 0 deletions utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ var modalities = require('./modalities')
var options = require('./options')
var type = require('./type')
const collectSummary = require('./summary/collectSummary')
const limit = require('./promise_limiter')

module.exports = {
array: array,
Expand All @@ -21,4 +22,5 @@ module.exports = {
options: options,
type: type,
collectSummary: collectSummary,
limit,
}
91 changes: 46 additions & 45 deletions utils/json.js
Original file line number Diff line number Diff line change
@@ -1,60 +1,61 @@
var Issue = require('./issues').Issue
var JSHINT = require('jshint').JSHINT
const Issue = require('./issues').Issue
const { JSHINT } = require('jshint')

module.exports = {
/**
* Parse
*
* Similar to native JSON.parse but uses
* a callback structure, jshint for more
* thorough error reporting and error formatting
* of the rest of the validator.
*/
parse: function(file, contents, callback) {
var jsObj = null
var err = null
/**
* Similar to native JSON.parse but returns a promise and
* runs jshint for more thorough error reporting
*/
function parse(file, contents) {
return new Promise(resolve => {
let jsObj
let err
try {
jsObj = JSON.parse(contents)
} catch (exception) {
err = exception
} finally {
if (err) {
this.jshint(file, contents, function(issues) {
callback(issues, null)
jshint(file, contents, function(issues) {
resolve({ issues, parsed: null })
})
} else {
callback([], jsObj)
resolve({ issues: [], parsed: jsObj })
}
}
},
})
}

/**
* JSHint
*
* Checks known invalid JSON file
* content in order to produce a
* verbose error message.
*/
jshint: function(file, contents, callback) {
var issues = []
if (!JSHINT(contents)) {
var out = JSHINT.data()
for (var i = 0; out.errors.length > i; ++i) {
var error = out.errors[i]
if (error) {
issues.push(
new Issue({
code: 27,
file: file,
line: error.line ? error.line : null,
character: error.character ? error.character : null,
reason: error.reason ? error.reason : null,
evidence: error.evidence ? error.evidence : null,
}),
)
}
/**
* JSHint
*
* Checks known invalid JSON file
* content in order to produce a
* verbose error message.
*/
function jshint(file, contents, callback) {
var issues = []
if (!JSHINT(contents)) {
var out = JSHINT.data()
for (var i = 0; out.errors.length > i; ++i) {
var error = out.errors[i]
if (error) {
issues.push(
new Issue({
code: 27,
file: file,
line: error.line ? error.line : null,
character: error.character ? error.character : null,
reason: error.reason ? error.reason : null,
evidence: error.evidence ? error.evidence : null,
}),
)
}
}
callback(issues)
},
}
callback(issues)
}

module.exports = {
parse,
jshint,
}
6 changes: 3 additions & 3 deletions utils/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,17 @@ module.exports = {
if (issues) {
callback(issues, null)
} else {
json.parse(file, contents, function(issues, jsObj) {
json.parse(file, contents).then(({ issues, parsed: jsObj }) => {
if (issues && issues.length > 0) {
callback(issues, null)
} else {
var parsed = {
const parsedConfig = {
ignore: jsObj.ignore ? jsObj.ignore : [],
warn: jsObj.warn ? jsObj.warn : [],
error: jsObj.error ? jsObj.error : [],
ignoredFiles: jsObj.ignoredFiles ? jsObj.ignoredFiles : [],
}
callback(null, parsed)
callback(null, parsedConfig)
}
})
}
Expand Down
6 changes: 6 additions & 0 deletions utils/promise_limiter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/* limits promises to LIMIT to prevent memory overuse */

const pLimit = require('p-limit')
const LIMIT = 200

module.exports = pLimit(LIMIT)
37 changes: 20 additions & 17 deletions validators/bval/validate.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,26 @@ const validate = (files, bContentsDict, annexed, dir) => {
let issues = []
// validate bval
const bvalPromises = files.map(function(file) {
return new Promise((resolve, reject) => {
utils.files
.readFile(file, annexed, dir)
.then(contents => {
bContentsDict[file.relativePath] = contents
bval(file, contents, function(bvalIssues) {
issues = issues.concat(bvalIssues)
resolve()
})
})
.catch(err =>
utils.issues.redirect(err, reject, () => {
issues.push(err)
resolve()
}),
)
})
return utils.limit(
() =>
new Promise((resolve, reject) => {
utils.files
.readFile(file, annexed, dir)
.then(contents => {
bContentsDict[file.relativePath] = contents
bval(file, contents, function(bvalIssues) {
issues = issues.concat(bvalIssues)
resolve()
})
})
.catch(err =>
utils.issues.redirect(err, reject, () => {
issues.push(err)
resolve()
}),
)
}),
)
})
return Promise.all(bvalPromises).then(() => issues)
}
Expand Down
37 changes: 20 additions & 17 deletions validators/bvec/validate.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,26 @@ const validate = (files, bContentsDict, annexed, dir) => {
// validate bvec
let issues = []
const bvecPromises = files.map(function(file) {
return new Promise((resolve, reject) => {
utils.files
.readFile(file, annexed, dir)
.then(contents => {
bContentsDict[file.relativePath] = contents
bvec(file, contents, function(bvecIssues) {
issues = issues.concat(bvecIssues)
resolve()
})
})
.catch(err =>
utils.issues.redirect(err, reject, () => {
issues.push(err)
resolve()
}),
)
})
return utils.limit(
() =>
new Promise((resolve, reject) => {
utils.files
.readFile(file, annexed, dir)
.then(contents => {
bContentsDict[file.relativePath] = contents
bvec(file, contents, function(bvecIssues) {
issues = issues.concat(bvecIssues)
resolve()
})
})
.catch(err =>
utils.issues.redirect(err, reject, () => {
issues.push(err)
resolve()
}),
)
}),
)
})
return Promise.all(bvecPromises).then(() => issues)
}
Expand Down
1 change: 1 addition & 0 deletions validators/json/json.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ const compareSidecarProperties = (file, sidecar) => {
return issues
}

// TODO: add /events.json schema loading/validation
const selectSchema = file => {
let schema = null
if (file.name) {
Expand Down
72 changes: 44 additions & 28 deletions validators/json/load.js
Original file line number Diff line number Diff line change
@@ -1,36 +1,52 @@
const utils = require('../../utils')

class JSONParseError extends Error {
constructor(message) {
super(message)
this.name = 'JSONParseError'
}
}

const load = (files, jsonFiles, jsonContentsDict, annexed, dir) => {
let issues = []
const jsonPromises = files.map(function(file) {
return new Promise((resolve, reject) => {
utils.files
.readFile(file, annexed, dir)
.then(contents => {
utils.json.parse(file, contents, function(parseIssues, jsObj) {
issues = issues.concat(parseIssues)

// abort further tests if schema test does not pass
if (issues.some(issue => issue.severity === 'error')) {
return reject()
}

jsonContentsDict[file.relativePath] = jsObj
jsonFiles.push(file)
resolve()
})
})
.catch(issue => {
issues.push(issue)
resolve()
})
})
})
return new Promise(resolve =>
Promise.all(jsonPromises)
.then(() => resolve(issues))
.catch(() => resolve(issues)),

// Read JSON file contents and parse for issues
const readJsonFile = (file, annexed, dir) =>
utils.files
.readFile(file, annexed, dir)
.then(contents => utils.json.parse(file, contents))
.then(({ issues: parseIssues, parsed }) => {
// Append any parse issues to returned issues
Array.prototype.push.apply(issues, parseIssues)

// Abort further tests if an error is found
if (
parseIssues &&
parseIssues.some(issue => issue.severity === 'error')
) {
throw new JSONParseError('Aborted due to parse error')
}

jsonContentsDict[file.relativePath] = parsed
jsonFiles.push(file)
})

// Start concurrent read/parses
const fileReads = files.map(file =>
utils.limit(() => readJsonFile(file, annexed, dir)),
)

// After all reads/parses complete, return any found issues
return Promise.all(fileReads)
.then(() => issues)
.catch(err => {
// Handle early exit
if (err instanceof JSONParseError) {
return issues
} else {
throw err
}
})
}

module.exports = load
Loading

0 comments on commit e5b3816

Please sign in to comment.