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.

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

Spam Error: TextEditor disposed #30578

Closed
bpasero opened this issue Jul 13, 2017 · 12 comments · Fixed by #31997
Closed

Spam Error: TextEditor disposed #30578

bpasero opened this issue Jul 13, 2017 · 12 comments · Fixed by #31997
Assignees
Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc.
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Jul 13, 2017

Our tests and also often the dev console are full of these errors:

Error: TextEditor disposed
    at MainThreadEditors.$trySetDecorations (file:///Users/travis/build/Microsoft/vscode/out/vs/workbench/api/electron-browser/mainThreadEditors.js:144:56)
    at MainThreadService.AbstractThreadService.handle (file:///Users/travis/build/Microsoft/vscode/out/vs/workbench/services/thread/common/abstractThreadService.js:25:27)
    at invokeHandler (file:///Users/travis/build/Microsoft/vscode/out/vs/platform/extensions/common/ipcRemoteCom.js:202:53)
    at receiveOneMessage (file:///Users/travis/build/Microsoft/vscode/out/vs/platform/extensions/common/ipcRemoteCom.js:166:36)
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickCallback (internal/process/next_tick.js:98:9)

I see no value in showing this error other than making it hard to read the test results to find an actual problem. This one seems to be ignored for many releases now so I suggest to remove it.

E.g.:

image

via https://travis-ci.org/Microsoft/vscode/jobs/253065123#L3436

@jrieken jrieken added the debt Code quality issues label Jul 13, 2017
@jrieken jrieken modified the milestone: July 2017 Jul 17, 2017
@jrieken jrieken assigned chrmarti and unassigned jrieken Jul 17, 2017
@jrieken
Copy link
Member

jrieken commented Jul 17, 2017

This is the merge decorator, add console.log('HERE', new Error().stack); to extHostEditor#setDecorations

HERE Error
    at ExtHostTextEditor.setDecorations (/Users/jrieken/Code/vscode/out/vs/workbench/api/node/file:/Users/jrieken/Code/vscode/src/vs/workbench/api/node/extHostTextEditor.ts:418:23)
    at __dirname.removeDecorations.Object.keys.forEach.decorationKey (/Users/jrieken/Code/vscode/extensions/merge-conflict/src/mergeDecorator.ts:248:12)
    at Array.forEach (native)
    at MergeDectorator.removeDecorations (/Users/jrieken/Code/vscode/extensions/merge-conflict/src/mergeDecorator.ts:241:33)
    at MergeDectorator.<anonymous> (/Users/jrieken/Code/vscode/extensions/merge-conflict/src/mergeDecorator.ts:185:10)
    at Generator.next (<anonymous>)
    at fulfilled (/Users/jrieken/Code/vscode/extensions/merge-conflict/out/mergeDecorator.js:4:58)

@joaomoreno joaomoreno added important Issue identified as high-priority and removed important Issue identified as high-priority labels Jul 28, 2017
@joaomoreno
Copy link
Member

It's incredibly hard to read travis logs right now.

@joaomoreno joaomoreno added engineering VS Code - Build / issue tracking / etc. important Issue identified as high-priority labels Jul 28, 2017
@chrmarti
Copy link
Contributor

@jrieken There is a check if the editor is in window.visibleTextEditors, but that doesn't seem to help. What is the correct way to check if the editor is not disposed?

@chrmarti
Copy link
Contributor

@jrieken Using console.log('HERE', this._disposed, new Error().stack); shows that this._disposed is still false when passing through extHostEditor#setDecorations. This seems to be a timing issue between extension host and main process.

@chrmarti
Copy link
Contributor

chrmarti commented Aug 2, 2017

extHostTextEditor already logs a warning in _runOnProxy() when the editor is disposed at the time setDecorations() is called. If the editor in the extension host is not disposed, but the one in the renderer process is (and the error is logged), the disposal and the setting of decorations were triggered at a similar time in different processes and it seems best to recover from this silently.

I suggest to remove the error. If an error is still desired, I'd suggest to change the warning in extHostTextEditor's _runOnProxy() to an error.

@chrmarti chrmarti removed their assignment Aug 2, 2017
@isidorn
Copy link
Contributor

isidorn commented Aug 3, 2017

@chrmarti could you please look into this since all other participants are on vacation? Or assign it to august if it is not critical for our relase

@chrmarti
Copy link
Contributor

chrmarti commented Aug 3, 2017

I don't see it as critical for the release. It is log entries for operations that should not run and also are not run. This is without further side-effects. @joaomoreno labelled it as important with regard to the noise in the test runs. I'll postpone it to August if there are no objections.

@chrmarti chrmarti modified the milestones: August 2017, July 2017 Aug 3, 2017
chrmarti added a commit that referenced this issue Aug 3, 2017
@jrieken jrieken reopened this Aug 14, 2017
@jrieken
Copy link
Member

jrieken commented Aug 14, 2017

Nice fix, not. The problem is explained here: #30578 (comment)

@jrieken jrieken removed their assignment Aug 14, 2017
@chrmarti
Copy link
Contributor

@jrieken This seems to be a race condition on the API's side. See #30578 (comment) .

@jrieken
Copy link
Member

jrieken commented Aug 14, 2017

This seems to be a race condition on the API's side

That might well be but the "fix" of this picking this one call is a little lame, isn't it? If we understand this problem why don't we tackle it for all other calls like tryRevealRange etc. Why don't we handle this properly in the _runOnProxy with a well known error code? This just looks like a shortcut to me

@chrmarti
Copy link
Contributor

We wanted to make progress on this "important" issue in your absence without devising a general solution without your input.

@jrieken
Copy link
Member

jrieken commented Aug 15, 2017

I understand that, but I would have just removed the "important" label because this is really just debt

@kieferrm kieferrm removed the important Issue identified as high-priority label Aug 15, 2017
jrieken added a commit that referenced this issue Aug 16, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants