Bugzilla – Full Text Bug Listing
| Summary: | Correctly report Tern errors when it fails to load | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Tomer Ohana <ohana54> | ||||||
| Component: | JS Tools | Assignee: | Michael Rennie <Michael_Rennie> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P2 | CC: | steve_northover, tomero | ||||||
| Version: | 14.0 | ||||||||
| Target Milestone: | 15.0 | ||||||||
| Hardware: | PC | ||||||||
| OS: | Mac OS X | ||||||||
| See Also: | https://github.com/eclipse/orion.client/pull/13 | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Tomer Ohana
Mike, please work with Tomer to get his pull request merged. Tomer, I noticed that he reformatted the file which is a no-no. If the file has bad formatting, do a commit that fixes the formatting only, then another commit that fixes the problem. This makes code changes easier to see and understand. Tomer, if you have not signed the Eclipse Committer Agreement, you need to do that for anything more than a few lines changes. https://eclipse.org/contribute/ Steve, I did the formatting change in a separate commit. The PR includes 2 commits, the formatting and the actual change. Regarding the ECA, that's weird because I remember signing it. I'll take care of it now. Apparently I have two users, I submitted the bug with my private user, and created the PR with my work user. Anyway, I signed the ECA with my work user, which is the same email in GitHub. Let me know if I need to do anything else. I'm sorry Tomer, I didn't look closely at the pull request. Anyhow, Mike owns the review for this. (In reply to Tomer Ohana from comment #0) > In ternWorkerCore.js, when Tern is loaded using the fallback option, it can > still fail if there is a parsing error due to a problem in a definition file. > The regular startup is wrapped with try-catch, but the fallback function is > not. > This will also cause a stack overflow because the startAndMessage function > eventually calls itself if it fails. > I will submit a PR that will try to solve it. Hi Tomer! Do you have some concrete steps to reproduce the problem? I was testing with a simple project (which I will attach) and when I introduce a typo in a definition - Tern does indeed fail to restart correctly - but the problem seems to be coming from the _getFile callback, and neither the old code nor your proposed fix seem to properly handle it. See the _loadDef function (~line 794): deferred.resolve(contents.length > 0 ? JSON.parse(contents) : Object.create(null)); when a def cannot be parsed the deferred is never resolved (or rejected) and the whole thing hangs waiting for it. I would propose we trap the exception here and drop the bad def from being loaded - that way we can continue on to load any other "good" definitions. Created attachment 268215 [details]
Test project
The project I was testing with.
Simply import it and open the Test.js file.
We encountered a case where the JSON was valid, but the Tern syntax was invalid so it failed (I'm sorry but I don't really remember what). In that case I think it should pass _loadDef and reach Tern. Another thing I notice now is that I made a mistake in the bug report, we are using v12 and not v14, but the code that I fixed looks the same in both versions. Note that we encountered the issue during development, with "hardcoded" definitions it obviously can't happen in production. If I remember correctly, we messed around with the definition file to test type inference with promises and made a typo. Example syntax: "fn() -> +Promise[value=+void]". I hope this helps :) (In reply to Tomer Ohana from comment #9) > We encountered a case where the JSON was valid, but the Tern syntax was > invalid so it failed (I'm sorry but I don't really remember what). In that > case I think it should pass _loadDef and reach Tern. > > Another thing I notice now is that I made a mistake in the bug report, we > are using v12 and not v14, but the code that I fixed looks the same in both > versions. > > Note that we encountered the issue during development, with "hardcoded" > definitions it obviously can't happen in production. > If I remember correctly, we messed around with the definition file to test > type inference with promises and made a typo. Example syntax: "fn() -> > +Promise[value=+void]". > > I hope this helps :) Thanks! I will try to make a test case for it. Created attachment 268251 [details]
Updated test project
Here is an updated test project with various typos in the definition file.
Tern still starts correctly for me, which is likely because we have made many fixes since v12 around Tern properly forwarding exceptions back to Orion when it fails to load a definition. We also fixed our definition type parser to ignore "bad stuff" in defs that can't be correctly parsed.
That's awesome! I think the PR is still good because it still fixes a potential stack overflow in case of an unforeseen error (fallback calling startAndMessage which can call fallback again...). WDYT? (In reply to Tomer Ohana from comment #12) > That's awesome! I think the PR is still good because it still fixes a > potential stack overflow in case of an unforeseen error (fallback calling > startAndMessage which can call fallback again...). > WDYT? I don't think it would hurt to pro-actively try and prevent overflows :) I think you will have to update the PR, as I released some code today that is causing conflicts (sorry about that). (In reply to Tomer Ohana from comment #12) > That's awesome! I think the PR is still good because it still fixes a > potential stack overflow in case of an unforeseen error (fallback calling > startAndMessage which can call fallback again...). > WDYT? Tomer, do you plan to update the PR so we can get it merged? Yes! I'm sorry, been swamped at work, I'll get to it in the next few days. Rebased the commits on current master, let me know if it's fine. (In reply to Tomer Ohana from comment #16) > Rebased the commits on current master, let me know if it's fine. Merged with master, thanks again Tomer! Awesome, glad to help |