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

Problems from main vscode build or extensions build can be shown, not both when a file is closed #119824

Open
roblourens opened this issue Mar 24, 2021 · 11 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug confirmed Issue has been confirmed by VS Code Team member tasks Task system issues
Milestone

Comments

@roblourens
Copy link
Member

Testing #119471

  • Follow steps in the linked item
  • Have compile errors in the extension build and also the main build
  • Open one of the files - the problems pane has errors from both builds
  • Close that file - the problems pane only has errors from one of the builds

Seems random which one it is

image

image

@alexr00 alexr00 added bug Issue identified by VS Code Team member as probable bug tasks Task system issues labels Mar 25, 2021
@alexr00 alexr00 added this to the Backlog milestone Mar 25, 2021
@meganrogge meganrogge self-assigned this May 17, 2022
@meganrogge meganrogge added the confirmed Issue has been confirmed by VS Code Team member label May 17, 2022
@meganrogge
Copy link
Contributor

for files when this doesn't work, it's returning here

if (!markerEvent.includes(modelEvent.uri) || (this.markerService.read({ resource: modelEvent.uri }).length !== 0)) {
return;
}

with the first of the conditions as true

@meganrogge
Copy link
Contributor

meganrogge commented May 17, 2022

The issue is that the schemes are different here - git vs file
Screen Shot 2022-05-17 at 12 16 07 PM

@meganrogge
Copy link
Contributor

meganrogge commented May 17, 2022

when it succeeds and restores the problems on close, the modelEvent has the correct scheme of file
Screen Shot 2022-05-17 at 12 18 58 PM

@meganrogge
Copy link
Contributor

meganrogge commented May 17, 2022

I think that check is actually working as designed since git resources are just redundant

@meganrogge
Copy link
Contributor

meganrogge commented May 17, 2022

This might be a part of the issue

case ApplyToKind.closedDocuments:
return !this.openModels[(await result.resource).toString()];
default:
return true;

@meganrogge
Copy link
Contributor

meganrogge commented May 18, 2022

I believe what's happening is a race condition between the listener of modelService.onModelRemoved in AbstractProblemCollector and that in its child, WatchingProblemCollector.

this.modelService.onModelRemoved((model) => {
delete this.openModels[model.uri.toString()];
}, this, this.modelListeners);

this.modelListeners.add(this.modelService.onModelRemoved(modelEvent => {
let markerChanged: IDisposable | undefined =
Event.debounce(this.markerService.onMarkerChanged, (last: readonly URI[] | undefined, e: readonly URI[]) => {
return (last ?? []).concat(e);
}, 500)(async (markerEvent) => {
markerChanged?.dispose();
markerChanged = undefined;
if (!markerEvent.includes(modelEvent.uri) || (this.markerService.read({ resource: modelEvent.uri }).length !== 0)) {
return;
}
const oldLines = Array.from(this.lines);
for (const line of oldLines) {
await this.processLineInternal(line);
}
});
setTimeout(async () => {
markerChanged?.dispose();
markerChanged = undefined;
}, 600);
}));

The onModelRemoved in WatchingProblemCollector calls processLineInternal which calls shouldApplyMatch, which depends upon upon the one in AbstractProblemCollector happening first to update openModels. Sometimes (often) that doesn't happen which results in shouldApplyMatch returning the wrong value so the matches don't get applied.

let shouldApplyMatch = await this.shouldApplyMatch(markerMatch);
if (shouldApplyMatch) {
this.recordMarker(markerMatch.marker, owner, resourceAsString);
if (this.currentOwner !== owner || this.currentResource !== resourceAsString) {
if (this.currentOwner && this.currentResource) {
this.deliverMarkersPerOwnerAndResource(this.currentOwner, this.currentResource);
}
this.currentOwner = owner;
this.currentResource = resourceAsString;
}
}

protected async shouldApplyMatch(result: ProblemMatch): Promise<boolean> {
switch (result.description.applyTo) {
case ApplyToKind.allDocuments:
return true;
case ApplyToKind.openDocuments:
return !!this.openModels[(await result.resource).toString()];
case ApplyToKind.closedDocuments:
return !this.openModels[(await result.resource).toString()];
default:
return true;
}
}

meganrogge added a commit that referenced this issue May 18, 2022
@meganrogge meganrogge modified the milestones: Backlog, May 2022 May 18, 2022
@meganrogge meganrogge changed the title Problems from main vscode build or extensions build can be shown, not both when a file is closed, problems often don't re-appear for it May 18, 2022
@meganrogge meganrogge changed the title when a file is closed, problems often don't re-appear for it Problems from main vscode build or extensions build can be shown, not both when a file is closed May 18, 2022
@meganrogge
Copy link
Contributor

meganrogge commented May 18, 2022

deliverMarkersPerOwnerAndResourceResolved isn't getting called when on file close the problems don't reappear for it

below the first half is the logs that happen when they do reappear and the second is when they don't

Screen Shot 2022-05-18 at 3 44 42 PM

@meganrogge
Copy link
Contributor

When I close a document and its errors don't get persisted, neither of these lines are getting hit
Screen Shot 2022-05-25 at 10 48 30 AM

@alexr00
Copy link
Member

alexr00 commented Jun 1, 2022

If I recall correctly, the problem stems from the fact that the VS Code build tasks only shows problems for closed files and we rely on the Typescript extension to provide problems for open files. To prevent duplicate problems in this setup (getting problems from two different sources) we use the same owner for both sets of problems. This is further complicated by there really being three sources for problems: the Typescript extension, the core build task, and the extension build task. The unfortunate side effect of this is that one or the other source can overwrite problems from the other. A real solution would require some kind of work in the markers service (see discussion starting from #124780 (comment)).

@alexr00 alexr00 modified the milestones: May 2022, June 2022 Jun 2, 2022
@alexr00
Copy link
Member

alexr00 commented Jun 2, 2022

Moving out of the May milestone.

@russelldavis
Copy link
Contributor

It doesn't fix the underlying issue, but there's a simple workaround for this for the vscode repo -- run the main build and the extension build as a single task. There's already an npm script for that (watchd) so it's a pretty simple change -- PR at #183174

russelldavis added a commit to russelldavis/vscode that referenced this issue May 23, 2023
russelldavis added a commit to russelldavis/vscode that referenced this issue Jun 9, 2023
russelldavis added a commit to russelldavis/vscode that referenced this issue Jul 18, 2023
russelldavis added a commit to russelldavis/vscode that referenced this issue Sep 30, 2023
russelldavis added a commit to russelldavis/vscode that referenced this issue Oct 2, 2023
russelldavis added a commit to russelldavis/vscode that referenced this issue Oct 3, 2023
russelldavis added a commit to russelldavis/vscode that referenced this issue Oct 7, 2023
russelldavis added a commit to russelldavis/vscode that referenced this issue Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug confirmed Issue has been confirmed by VS Code Team member tasks Task system issues
Projects
None yet
Development

No branches or pull requests

4 participants