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.

Sign up for GitHub

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

Jump to bottom

[CLOSED] Fix #5137 async linting #5935

Open
core-ai-bot opened this issue Aug 30, 2021 · 27 comments
Open

[CLOSED] Fix #5137 async linting #5935

core-ai-bot opened this issue Aug 30, 2021 · 27 comments

Comments

@core-ai-bot
Copy link
Member

Issue by busykai
Wednesday Jan 15, 2014 at 15:36 GMT
Originally opened as adobe/brackets#6530

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

busykai included the following code: https://github.com/adobe/brackets/pull/6530/commits
@core-ai-bot
Copy link
Member Author

Comment by busykai
Wednesday Jan 15, 2014 at 15:38 GMT

Linking to discussion in #5137.

@core-ai-bot
Copy link
Member Author

Comment by busykai
Wednesday Jan 15, 2014 at 16:57 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by ingorichter
Wednesday Jan 15, 2014 at 23:02 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by busykai
Wednesday Jan 15, 2014 at 23:05 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by busykai
Wednesday Feb 26, 2014 at 12:46 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by njx
Wednesday Feb 26, 2014 at 21:25 GMT

Looks like@peterflynn is currently assigned to this.

@core-ai-bot
Copy link
Member Author

Comment by ingorichter
Wednesday Mar 05, 2014 at 17:57 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by busykai
Thursday Mar 06, 2014 at 02:10 GMT

@ingorichter, great! thank you!

@core-ai-bot
Copy link
Member Author

Comment by ingorichter
Friday Mar 07, 2014 at 17:56 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by fdecampredon
Saturday Mar 08, 2014 at 19:42 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by busykai
Monday Mar 17, 2014 at 14:50 GMT

@ingorichter made the changes as per review.

@core-ai-bot
Copy link
Member Author

Comment by busykai
Monday Mar 17, 2014 at 14:59 GMT

@fdecampredon, what about an interface which would allow you to enable/disable your provider? i was trying to think of a generic API to support provider lifecycle like yours, but then it gets too complicated when trying to embrace all possible conditions when a provider may want to go offline for a while. instead, a much simple approach is to give the provider a way to indicate when it's ready and when it's not.

@core-ai-bot
Copy link
Member Author

Comment by ingorichter
Monday Mar 17, 2014 at 15:44 GMT

I wrote a unit test for the race condition and we need to check, if the document is still the same when the inspection/linting results will be displayed. My case was to open a document, start inspection, close document and the results for the previous document will show up in the bottom panel. This shouldn't be too difficult to avoid for us.

@core-ai-bot
Copy link
Member Author

Comment by busykai
Monday Mar 17, 2014 at 16:09 GMT

@ingorichter, that's weird. i had this case in mind. isn't it what line 312 does already? it's done when Async.doInParallel is settled. could you push your testcase here so i can work on it? scratch that last one, it's actually a PR from my previous life.

@core-ai-bot
Copy link
Member Author

Comment by fdecampredon
Tuesday Mar 18, 2014 at 00:41 GMT

@busykai yep enabling/disabling could be a cool way of handling inspector like mine, especially if a lint session is started when a linter get enabled.

@core-ai-bot
Copy link
Member Author

Comment by ingorichter
Wednesday Mar 19, 2014 at 23:23 GMT

@busykai would you mind adding this https://gist.github.com/busykai/9605238 test case to the existing ones?

@core-ai-bot
Copy link
Member Author

Comment by ingorichter
Wednesday Mar 19, 2014 at 23:41 GMT

Resolving the promise with a message, that the linter was unable to return something in a timely manner is not implemented yet. I wondered, what this means for the user and what the user is supposed to do in such a situation. I don't have a good idea how to resolve this in an elegant way. The next time the user makes a changes to the file and we start a new linting attempt this issue could be resolved, or not. I hope that the user doesn't get annoyed when this happens too frequently (since this is completely new behavior).

@core-ai-bot
Copy link
Member Author

Comment by ingorichter
Wednesday Mar 19, 2014 at 23:41 GMT

@busykai I done with my review for now.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Friday Mar 21, 2014 at 00:29 GMT

@busykai Done reviewing the code -- looks pretty clean.

My biggest concern, however, is it doesn't look like there's any attempt here to handle the myriad of new race conditions that this change creates. For example, if I hit Save in a file where the linter takes 1 sec to complete and then I switch to another file, the linting results from the old file will pop up while I'm in the new file. If the new file had its own linter, and it was faster, you'll actually have a flash of the correct results and then they'll get overwritten with incorrect results from the old file. Similar things can happen if you disable linting while one is still in progress, etc. Bugs are even possible just from editing within a single file: you can stack up multiple invocations of the async provider, and they might finish out of order, showing you stale linting results from one of the earlier invocations.

This isn't something we can solve in inspectFile() -- rather, these are really all issues with run()'s usage of it, and so I think the fixes belong there. Previously, run() was basically able to assume that inspectFile() completes synchronously; now it can't assume that.

We should also add some more unit tests to cover a bunch of these tricky conditions.

@core-ai-bot
Copy link
Member Author

Comment by busykai
Monday Mar 31, 2014 at 02:40 GMT

@peterflynn, actually many of the cases you describe are, to my knowledge, not possible. similar case was brought up by@ingorichter. by design, run always holds the last master promise from the inspectFile and will not react to any previous promises which might be in flight and resolved later. I've added test case which illustrates that (based on what@ingorichter was showing me). It's here: a329181.

I'm done with addressing the previous review comments. It's ready for review again.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Monday Mar 31, 2014 at 07:44 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by busykai
Monday Mar 31, 2014 at 23:43 GMT

@peterflynn, changed the check as per your suggestion.

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

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Wednesday Apr 02, 2014 at 23:05 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by busykai
Friday Apr 04, 2014 at 21:57 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Tuesday Apr 08, 2014 at 04:59 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Tuesday Apr 08, 2014 at 19:08 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by busykai
Wednesday Apr 09, 2014 at 12:32 GMT

Yay! Great! Now to improve it! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot