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

Conversation

busykai
Copy link
Contributor

@busykai busykai commented Jan 15, 2014

Implement support for asynchronous providers through scanFileAsync function. Each provider is limited by 500ms timeout now. Providers are executed in parallel (even though it only makes sense for the truly async ones). Providers are never rejected if actual provider rejects its promised, it is resolve with null results and does not appear in the problems panel.

Additionally some closure compiler notation issues in JSDoc have been fixed.

cc: @peterflynn, @ingorichter

Not working due to an issue in code inspection implementation (html
template is displayed before it's rendered).
…nting

Applied experimental version as the base for the development.

Conflicts:
	src/language/CodeInspection.js
Use runPromise as the aggregation point instead of doing it in varios
places.
@busykai
Copy link
Contributor Author

busykai commented Jan 15, 2014

Linking to discussion in #5137.

@busykai
Copy link
Contributor Author

busykai commented Jan 15, 2014

For the brave: checkout this async JSHint implementation if you want to test (checkout use-async-run branch, specifically). You need to place it somewhere where the extensions are loaded from, either in src/extensions/dev or in <AppSupportDirectory>/extensions/user.

@ghost ghost assigned peterflynn Jan 15, 2014
@ingorichter
Copy link
Contributor

@busykai I have a bunch of other stuff to do before we finish our sprint and I don't believe that I can pull this in, since we decided to focus on the new preferences system and the filesystem issues first. But I'm going to review this for Sprint 37.

@busykai
Copy link
Contributor Author

busykai commented Jan 15, 2014

@ingorichter that's fine, this was pointing at Sprint 37. I understand the timing. Unit tests are missing too, like you rightfully mentioned. :)

@busykai
Copy link
Contributor Author

busykai commented Feb 26, 2014

@ingorichter, would you have a chance to look at it anytime soon?

@njx
Copy link
Contributor

njx commented Feb 26, 2014

Looks like @peterflynn is currently assigned to this.

…nting

Conflicts:
	src/language/CodeInspection.js
@ingorichter
Copy link
Contributor

@busykai Sorry that it took so long for us to get to this PR. I'll look at it today.

@busykai
Copy link
Contributor Author

busykai commented Mar 6, 2014

@ingorichter, great! thank you!

@ingorichter
Copy link
Contributor

I'm currently testing some edge cases. I hope to be finished by today.

@fdecampredon
Copy link

Like I said to Peter Flynn on IRC, I develop my extension (TypeScript language support) using the code of this PR . My linter runs in a Web Worker and everything works fine except that when I open brackets with a TypeScript files open I got the 2 following errors in the chrome console :

Recursive tests with the same name are not supported. Timer name: CodeInspection:   
/Users/kapit/Documents/workspaces/typescript/brackets-typescript/src/commons/completion.ts PerfUtils.js:116
Recursive tests with the same name are not supported. Timer name: CodeInspection 'TypeScript':  
/Users/kapit/Documents/workspaces/typescript/brackets-typescript/src/commons/completion.ts PerfUtils.js:116

I guess that at start times if the linter takes a little times to answer (that is the case here since the typescript compiler takes a little time to load and parse all the files of the current project) 2 inspection are launch in parallel resulting in these errors.

};
spyOn(provider, "scanFileAsync").andCallThrough();

if (syncImpl === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

little nit: if (syncImpl) { should be sufficient here.

@peterflynn
Copy link
Member

@busykai Ah, I think I see what's going on: there's existing code in run() that bails if the current file has changed since the linting was kicked off. But there's nothing more generalized to ensure we ignore all stale promises, so the other race conditions I listed above could still happen. I can post some failing testcases if it'd be helpful.

Here's one idea for a fix: have a module-global variable that holds the "current" pending promise. Overwrite it whenever a new run() is initiated (overwrite with null if !_enabled). In the then() handler, check if the promise is still the current one and if not, bail. That could replace the current-document check code since it's essentially a superset of it.

Keep track of the last promise which inspectFile returns to prevent any
stale promises from updating the UI.
@busykai
Copy link
Contributor Author

busykai commented Mar 31, 2014

@peterflynn, changed the check as per your suggestion.

@ingorichter, this is ready for another round of review.

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).

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.

results.sort(function (a, b) {
// actual provider list may have changed in the process, but we don't care
return _.indexOf(providerList, a.provider) - _.indexOf(providerList, b.provider);
});
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not do this in inspectFile() instead, so all clients get consistently sorted results like they have been getting previous sprints?

Also, I'm a little confused about the "provider list may have changed in the process" - you mean during the async delay, new providers may have been registered?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I think a simple providerList.indexOf() would work fine 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.

@peterflynn, I guess my line of thinking was not to sort it before there's a chance it can be discarded. will change to use Array's indexOf.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you mean if the promise is stale? I think the perf hit of sorting discarded linting results is pretty negligible -- on balance, it seems better to have the API do the right thing for all clients instead of making everyone rewrite this piece of code.

@peterflynn
Copy link
Member

@busykai Done with re-review. Just a bunch of small things at this point I think.

- Sort the results immediately in inspectFile.
- Refactor the tests. Set timeout low for the tests to run faster.
- Increase timeout to 10 seconds.
@busykai
Copy link
Contributor Author

busykai commented Apr 4, 2014

@peterflynn done with addressing comments. please take a look.

@peterflynn
Copy link
Member

@busykai This looks great! -- nothing left that should really hold up merging. I'm just going to do a little hand-testing tonight before pushing the button...

@peterflynn
Copy link
Member

Merging now. Thanks for all your work on this @busykai!

peterflynn added a commit that referenced this pull request Apr 8, 2014
Support linting providers that return results asynchronously
@peterflynn peterflynn merged commit 9111c08 into adobe:master Apr 8, 2014
@busykai
Copy link
Contributor Author

busykai commented Apr 9, 2014

Yay! Great! Now to improve it! :)

@busykai busykai deleted the fix-5137-async-linting branch April 9, 2014 12:32
peterflynn added a commit that referenced this pull request Apr 25, 2014
- Fix bug: if one linter returns errors and another returns errors:[], the
no-errors linter is shown in the panel as an empty heading. (Bug doesn't
happen if it returned null instead; but both are valid ways of indicating
zero errors).
- Improve docs on register() - more concise, more detail on how to indicate
zero errors
- Add unit tests for the bug fixed above plus several more async linting
cases that were fixed late in PR #6530
- Fix mislabeled existing unit test for reject()ing async linters
- Fix stray whitespace that snuck into merge c3411bb
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants