Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fix #5137 async linting #6530

Merged
merged 22 commits into from
Apr 8, 2014
Merged

Fix #5137 async linting #6530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
835b3af
Bare async provider implementation.
busykai Jan 3, 2014
db33c0f
Merge remote-tracking branch 'origin/fix-5137' into fix-5137-async-li…
busykai Jan 12, 2014
e730595
Fix @param specification error.
busykai Jan 12, 2014
d9deaa1
Change documentation and JSDoc.
busykai Jan 15, 2014
b91ec3f
Put timeout on provider execution.
busykai Jan 15, 2014
08e0392
Change to record notation (understood by CC).
busykai Jan 15, 2014
03c9ed3
Fix typo in the docs.
busykai Jan 15, 2014
5ec5534
Resolve with null on timeout. Lint.
busykai Jan 16, 2014
9fc0425
Add unit tests.
busykai Jan 16, 2014
62933ce
Merge remote-tracking branch 'upstream/master' into fix-5137-async-li…
busykai Feb 26, 2014
d684e93
Merge remote-tracking branch 'upstream/master' into fix-5137-async-li…
busykai Mar 17, 2014
0c091df
Address code review comments.
busykai Mar 17, 2014
fe7d342
Increase timeout to 1000ms and make it a pref.
busykai Mar 17, 2014
a329181
Add test case for race condition.
busykai Mar 27, 2014
7435877
Address nits and documentation review outcome.
busykai Mar 27, 2014
fecb543
Resolve with an error on timeout/failure.
busykai Mar 30, 2014
563263d
Merge branch 'master' into fix-5137-async-linting
busykai Mar 30, 2014
6f5e030
Keep the order of the providers when reporting.
busykai Mar 31, 2014
c786e69
Check for current promise instead of document.
busykai Mar 31, 2014
a075ab4
Remove left-over documentation sentence.
busykai Apr 2, 2014
a46b9df
Address review comments.
busykai Apr 4, 2014
c3411bb
Merge remote-tracking branch 'upstream/master' into fix-5137-async-li…
busykai Apr 5, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 94 additions & 31 deletions src/language/CodeInspection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ define(function (require, exports, module) {
* Constants for the preferences defined in this file.
*/
var PREF_ENABLED = "enabled",
PREF_COLLAPSED = "collapsed";
PREF_COLLAPSED = "collapsed",
PREF_ASYNC_TIMEOUT = "asyncTimeout";

var prefs = PreferencesManager.getExtensionPrefs("linting");

Expand Down Expand Up @@ -120,7 +121,7 @@ define(function (require, exports, module) {

/**
* @private
* @type {Object.<languageId:string, Array.<{name:string, scanFile:function(string, string):Object}>>}
* @type {{languageId:string, Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):Object}>}}
*/
var _providers = {};

Expand All @@ -129,6 +130,15 @@ define(function (require, exports, module) {
* @type {boolean}
*/
var _hasErrors;

/**
* Promise of the returned by the last call to inspectFile or null if linting is disabled. Used to prevent any stale promises
* to cause updates of the UI.
*
* @private
* @type {$.Promise}
*/
var _currentPromise = null;

/**
* Enable or disable the "Go to First Error" command
Expand All @@ -148,7 +158,7 @@ define(function (require, exports, module) {
* Decision is made depending on the file extension.
*
* @param {!string} filePath
* @return ?{Array.<{name:string, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}>} provider
* @return ?{Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):?{errors:!Array, aborted:boolean}}>} provider
*/
function getProvidersForPath(filePath) {
return _providers[LanguageManager.getLanguageForPath(filePath).getId()];
Expand All @@ -166,7 +176,7 @@ define(function (require, exports, module) {
* If there are no providers registered for this file, the Promise yields null instead.
*
* @param {!File} file File that will be inspected for errors.
* @param {?Array.<{name:string, scanFile:function(string, string):?{errors:!Array, aborted:boolean}}>} providerList
* @param {?Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):?{errors:!Array, aborted:boolean}}>} providerList
* @return {$.Promise} a jQuery promise that will be resolved with ?Array.<{provider:Object, result: ?{errors:!Array, aborted:boolean}}>
*/
function inspectFile(file, providerList) {
Expand All @@ -179,29 +189,72 @@ define(function (require, exports, module) {
response.resolve(null);
return response.promise();
}

DocumentManager.getDocumentText(file)
.done(function (fileText) {
var perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + file.fullPath);

providerList.forEach(function (provider) {
var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + file.fullPath);

try {
var scanResult = provider.scanFile(fileText, file.fullPath);
var perfTimerInspector = PerfUtils.markStart("CodeInspection:\t" + file.fullPath),
masterPromise;

masterPromise = Async.doInParallel(providerList, function (provider) {
Copy link
Member

Choose a reason for hiding this comment

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

doInParallel() means the order of the providers in the results table will be nondeterministic. So this should either be sorted before display (e.g. alphabetically by provider name), or it should use doSequentially() so the order is stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Added a test case for it also.

var perfTimerProvider = PerfUtils.markStart("CodeInspection '" + provider.name + "':\t" + file.fullPath),
runPromise = new $.Deferred();

runPromise.done(function (scanResult) {
results.push({provider: provider, result: scanResult});
} catch (err) {
console.error("[CodeInspection] Provider " + provider.name + " threw an error: " + err);
response.reject(err);
return;
});

if (provider.scanFileAsync) {
window.setTimeout(function () {
// timeout error
var errTimeout = {
pos: { line: -1, col: 0},
message: StringUtils.format(Strings.LINTER_TIMED_OUT, provider.name, prefs.get(PREF_ASYNC_TIMEOUT)),
type: Type.ERROR
};
runPromise.resolve({errors: [errTimeout]});
}, prefs.get(PREF_ASYNC_TIMEOUT));
Copy link
Member

Choose a reason for hiding this comment

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

I still think we should remove this timeout -- it breaks use cases like retrieving linting results from a web service, and it's something we don't do in any of our other async provider-based APIs. And I don't see a clear reason why we need it (especially with the race condition guard that makes us ignore stale promises, even if they dangle forever).

If we must keep it, we should make the default timeout significantly longer and use Async.withTimeout() to simplify the code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current model, any hanging request actually delays the entire rendering. I agree with you and Ingo that this is a fragile with regards to the linters which may take long, but it could be addressed incrementally (e.g. make results rendering incremental without re-running all the linters). It should not be holding up this PR. This is already way better than nothing. Existing linters can take advantage of this. This is all started just because I wanted to support recursive configuration file look up in JSHint which can be done right now. I don't see this is piece of code as a step in an opposite direction.

Copy link
Member

Choose a reason for hiding this comment

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

I understand those are your needs, but there are multiple other people who've requested this specifically in order to get web services or node-side services into the picture. I don't think we can call #5137 fixed if we're timing out after just 1 sec. How about we just raise it to 5 or 10 sec for now?

It also seems weird that this is a single global pref rather than an option linters can specify when registering, or a per-linter pref. I think we'll need to revisit that if there are complaints about the default timeout, but for now it seems ok (as long as we bump up the default here).

Copy link

Choose a reason for hiding this comment

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

In my case like I explained on IRC, my initial lint can take few seconds on large projects (while the subsequent will be faster). So if you go with the timeout a bigger one would make me happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Let's make it 10 secs for now. I guess my PoV was limited to the uses cases I was exposed to. I filed #7397 which is the right way to deal with this entire timeout issue.

provider.scanFileAsync(fileText, file.fullPath)
.done(function (scanResult) {
PerfUtils.addMeasurement(perfTimerProvider);
runPromise.resolve(scanResult);
})
.fail(function (err) {
var errError = {
pos: {line: -1, col: 0},
message: StringUtils.format(Strings.LINTER_FAILED, provider.name, err),
type: Type.ERROR
};
console.error("[CodeInspection] Provider " + provider.name + " (async) failed: " + err);
runPromise.resolve({errors: [errError]});
});
} else {
try {
var scanResult = provider.scanFile(fileText, file.fullPath);
PerfUtils.addMeasurement(perfTimerProvider);
runPromise.resolve(scanResult);
} catch (err) {
var errError = {
pos: {line: -1, col: 0},
message: StringUtils.format(Strings.LINTER_FAILED, provider.name, err),
type: Type.ERROR
};
console.error("[CodeInspection] Provider " + provider.name + " (sync) threw an error: " + err);
runPromise.resolve({errors: [errError]});
}
}
return runPromise.promise();

PerfUtils.addMeasurement(perfTimerProvider);
}, false);

masterPromise.then(function () {
// sync async may have pushed results in different order, restore the original order
results.sort(function (a, b) {
return providerList.indexOf(a.provider) - providerList.indexOf(b.provider);
});
PerfUtils.addMeasurement(perfTimerInspector);
response.resolve(results);
});

PerfUtils.addMeasurement(perfTimerInspector);

response.resolve(results);
})
.fail(function (err) {
console.error("[CodeInspection] Could not read file for inspection: " + file.fullPath);
Expand All @@ -216,7 +269,7 @@ define(function (require, exports, module) {
* change based on the number of problems reported and how many provider reported problems.
*
* @param {Number} numProblems - total number of problems across all providers
* @param {Array.<{name:string, scanFile:function(string, string):Object}>} providersReportingProblems - providers that reported problems
* @param {Array.<{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):Object}>} providersReportingProblems - providers that reported problems
* @param {boolean} aborted - true if any provider returned a result with the 'aborted' flag set
*/
function updatePanelTitleAndStatusBar(numProblems, providersReportingProblems, aborted) {
Expand Down Expand Up @@ -261,6 +314,7 @@ define(function (require, exports, module) {
function run() {
if (!_enabled) {
_hasErrors = false;
_currentPromise = null;
Resizer.hide($problemsPanel);
StatusBar.updateIndicator(INDICATOR_ID, true, "inspection-disabled", Strings.LINT_DISABLED);
setGotoEnabled(false);
Expand All @@ -278,12 +332,12 @@ define(function (require, exports, module) {
var providersReportingProblems = [];

// run all the providers registered for this file type
inspectFile(currentDoc.file, providerList).then(function (results) {
// check if current document wasn't changed while inspectFile was running
if (currentDoc !== DocumentManager.getCurrentDocument()) {
(_currentPromise = inspectFile(currentDoc.file, providerList)).then(function (results) {
// check if promise has not changed while inspectFile was running
if (this !== _currentPromise) {
return;
}

// how many errors in total?
var errors = results.reduce(function (a, item) { return a + (item.result ? item.result.errors.length : 0); }, 0);

Expand Down Expand Up @@ -361,6 +415,7 @@ define(function (require, exports, module) {
} else {
// No provider for current file
_hasErrors = false;
_currentPromise = null;
Resizer.hide($problemsPanel);
var language = currentDoc && LanguageManager.getLanguageForPath(currentDoc.file.fullPath);
if (language) {
Expand All @@ -380,9 +435,16 @@ define(function (require, exports, module) {
* Registering any provider for the "javascript" language automatically unregisters the built-in
* Brackets JSLint provider. This is a temporary convenience until UI exists for disabling
* registered providers.
*
* If provider implements both scanFileAsync and scanFile functions for asynchronous and synchronous
* code inspection, respectively, the asynchronous version will take precedence and will be used to
* perform code inspection.
*
* A code inspection provider's scanFileAsync must return a {$.Promise} object which should be
* resolved with ?{errors:!Array, aborted:boolean}}.
*
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add a sentence that makes it clear, that the scanFileAsync always has precedence over scanFile iof both of them would have been provided. I think this is an edge case, but we make it clear right from the beginning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow I added this note to inspectFile() documentation. :) Will add clarification here as well.

* @param {string} languageId
* @param {{name:string, scanFile:function(string, string):?{errors:!Array, aborted:boolean}} provider
* @param {{name:string, scanFileAsync:?function(string, string):!{$.Promise}, scanFile:?function(string, string):?{errors:!Array, aborted:boolean}}} provider
*
* Each error is: { pos:{line,ch}, endPos:?{line,ch}, message:string, type:?Type }
* If type is unspecified, Type.WARNING is assumed.
Expand All @@ -391,15 +453,15 @@ define(function (require, exports, module) {
if (!_providers[languageId]) {
_providers[languageId] = [];
}

if (languageId === "javascript") {
// This is a special case to enable extension provider to replace the JSLint provider
// in favor of their own implementation
_.remove(_providers[languageId], function (registeredProvider) {
return registeredProvider.name === "JSLint";
});
}

_providers[languageId].push(provider);

run(); // in case a file of this type is open currently
Expand Down Expand Up @@ -508,7 +570,7 @@ define(function (require, exports, module) {
toggleCollapsed(prefs.get(PREF_COLLAPSED), true);
});


prefs.definePreference(PREF_ASYNC_TIMEOUT, "number", 10000);

// Initialize items dependent on HTML DOM
AppInit.htmlReady(function () {
Expand Down Expand Up @@ -571,12 +633,13 @@ define(function (require, exports, module) {
});

// Testing
exports._unregisterAll = _unregisterAll;
exports._unregisterAll = _unregisterAll;
exports._PREF_ASYNC_TIMEOUT = PREF_ASYNC_TIMEOUT;

// Public API
exports.register = register;
exports.Type = Type;
exports.toggleEnabled = toggleEnabled;
exports.inspectFile = inspectFile;
exports.requestRun = run;
exports.requestRun = run;
});
3 changes: 3 additions & 0 deletions src/nls/root/strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ define({
"FILE_FILTER_INSTRUCTIONS" : "Exclude files and folders matching any of the following strings / substrings or <a href='{0}' title='{0}'>wildcards</a>. Enter each string on a new line.",
"FILE_FILTER_LIST_PREFIX" : "except",
"FILE_FILTER_CLIPPED_SUFFIX" : "and {0} more",

"FILTER_COUNTING_FILES" : "Counting files\u2026",
"FILTER_FILE_COUNT" : "Allows {0} of {1} files {2}",
"FILTER_FILE_COUNT_ALL" : "Allows all {0} files {1}",
Expand Down Expand Up @@ -239,6 +240,8 @@ define({
"LINT_DISABLED" : "Linting is disabled",
"NO_LINT_AVAILABLE" : "No linter available for {0}",
"NOTHING_TO_LINT" : "Nothing to lint",
"LINTER_TIMED_OUT" : "{0} has timed out after waiting for {1} ms",
"LINTER_FAILED" : "{0} terminated with error: {1}",


/**
Expand Down
Loading