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

improve queues, (one effect: improve code completion) #4310

Merged
merged 8 commits into from
Jan 4, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions src/observers/OmnisharpDebugModeLoggerObserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { BaseLoggerObserver } from "./BaseLoggerObserver";
import * as os from 'os';
import { BaseEvent, OmnisharpRequestMessage, OmnisharpServerEnqueueRequest, OmnisharpServerDequeueRequest, OmnisharpServerVerboseMessage, OmnisharpServerProcessRequestStart, OmnisharpEventPacketReceived } from "../omnisharp/loggingEvents";
import { BaseEvent, OmnisharpRequestMessage, OmnisharpServerEnqueueRequest, OmnisharpServerDequeueRequest, OmnisharpServerRequestCancelled, OmnisharpServerVerboseMessage, OmnisharpServerProcessRequestStart, OmnisharpEventPacketReceived } from "../omnisharp/loggingEvents";
import { EventType } from "../omnisharp/EventType";

export class OmnisharpDebugModeLoggerObserver extends BaseLoggerObserver {
Expand All @@ -20,6 +20,9 @@ export class OmnisharpDebugModeLoggerObserver extends BaseLoggerObserver {
case EventType.OmnisharpServerDequeueRequest:
this.handleOmnisharpServerDequeueRequest(<OmnisharpServerDequeueRequest>event);
break;
case EventType.OmnisharpServerRequestCancelled:
this.handleOmnisharpServerRequestCancelled(<OmnisharpServerRequestCancelled>event);
break;
case EventType.OmnisharpServerProcessRequestStart:
this.handleOmnisharpProcessRequestStart(<OmnisharpServerProcessRequestStart>event);
break;
Expand All @@ -44,17 +47,22 @@ export class OmnisharpDebugModeLoggerObserver extends BaseLoggerObserver {
}

private handleOmnisharpServerEnqueueRequest(event: OmnisharpServerEnqueueRequest) {
this.logger.appendLine(`Enqueue ${event.name} request for ${event.command}.`);
this.logger.appendLine(`Enqueue to ${event.queueName} request for ${event.command}.`);
this.logger.appendLine();
}

private handleOmnisharpServerDequeueRequest(event: OmnisharpServerDequeueRequest) {
this.logger.appendLine(`Dequeue ${event.name} request for ${event.command} (${event.id}).`);
this.logger.appendLine(`Dequeue from ${event.queueName} with status ${event.queueStatus} request for ${event.command}${event.id ? ` (${event.id})` : ''}.`);
this.logger.appendLine();
}

private handleOmnisharpServerRequestCancelled(event: OmnisharpServerRequestCancelled) {
this.logger.appendLine(`Cancelled request for ${event.command} (${event.id}).`);
this.logger.appendLine();
}

private handleOmnisharpProcessRequestStart(event: OmnisharpServerProcessRequestStart) {
this.logger.appendLine(`Processing ${event.name} queue`);
this.logger.appendLine(`Processing ${event.name} queue, available slots ${event.availableRequestSlots}`);
this.logger.increaseIndent();
}

Expand Down
3 changes: 2 additions & 1 deletion src/omnisharp/EventType.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ export enum EventType {
ProjectDiagnosticStatus = 75,
DotNetTestRunInContextStart = 76,
DotNetTestDebugInContextStart = 77,
TelemetryErrorEvent = 78
TelemetryErrorEvent = 78,
OmnisharpServerRequestCancelled = 79,
}

//Note that the EventType protocol is shared with Razor.VSCode and the numbers here should not be altered
Expand Down
11 changes: 8 additions & 3 deletions src/omnisharp/loggingEvents.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,17 +115,22 @@ export class OmnisharpServerUnresolvedDependencies implements BaseEvent {

export class OmnisharpServerEnqueueRequest implements BaseEvent {
type = EventType.OmnisharpServerEnqueueRequest;
constructor(public name: string, public command: string) { }
constructor(public queueName: string, public command: string) { }
}

export class OmnisharpServerDequeueRequest implements BaseEvent {
type = EventType.OmnisharpServerDequeueRequest;
constructor(public name: string, public command: string, public id: number) { }
constructor(public queueName: string, public queueStatus: string, public command: string, public id?: number) { }
}

export class OmnisharpServerRequestCancelled implements BaseEvent {
type = EventType.OmnisharpServerRequestCancelled;
constructor(public command: string, public id: number) { }
}

export class OmnisharpServerProcessRequestStart implements BaseEvent {
type = EventType.OmnisharpServerProcessRequestStart;
constructor(public name: string) { }
constructor(public name: string, public availableRequestSlots: number) { }
}

export class OmnisharpEventPacketReceived implements BaseEvent {
Expand Down
3 changes: 2 additions & 1 deletion src/omnisharp/prioritization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ const priorityCommands = [
];

const normalCommands = [
protocol.Requests.AutoComplete,
protocol.Requests.Completion,
protocol.Requests.CompletionResolve,
protocol.Requests.FilesChanged,
protocol.Requests.FindSymbols,
protocol.Requests.FindUsages,
Expand Down
1 change: 0 additions & 1 deletion src/omnisharp/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import { CompletionTriggerKind, CompletionItemKind, CompletionItemTag, InsertTex

export module Requests {
export const AddToProject = '/addtoproject';
export const AutoComplete = '/autocomplete';
export const CodeCheck = '/codecheck';
export const CodeFormat = '/codeformat';
export const ChangeBuffer = '/changebuffer';
Expand Down
18 changes: 9 additions & 9 deletions src/omnisharp/requestQueue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export interface Request {
onError(err: any): void;
startTime?: number;
endTime?: number;
id?: number;
}

/**
Expand Down Expand Up @@ -47,7 +48,7 @@ class RequestQueue {

if (request) {
this._waiting.delete(id);
this.eventStream.post(new OmnisharpServerDequeueRequest(this._name, request.command, id));
this.eventStream.post(new OmnisharpServerDequeueRequest(this._name, "waiting", request.command, id));
}

return request;
Expand All @@ -57,12 +58,12 @@ class RequestQueue {
let index = this._pending.indexOf(request);
if (index !== -1) {
this._pending.splice(index, 1);

// Note: This calls reject() on the promise returned by OmniSharpServer.makeRequest
request.onError(new Error(`Pending request cancelled: ${request.command}`));
this.eventStream.post(new OmnisharpServerDequeueRequest(this._name, "pending", request.command));
}

// TODO: Handle cancellation of a request already waiting on the OmniSharp server.
if (request.id){
this.dequeue(request.id);
}
}

/**
Expand All @@ -87,11 +88,10 @@ class RequestQueue {
return;
}

this.eventStream.post(new OmnisharpServerProcessRequestStart(this._name));

const slots = this._maxSize - this._waiting.size;
const availableRequestSlots = this._maxSize - this._waiting.size;
this.eventStream.post(new OmnisharpServerProcessRequestStart(this._name, availableRequestSlots));

for (let i = 0; i < slots && this._pending.length > 0; i++) {
for (let i = 0; i < availableRequestSlots && this._pending.length > 0; i++) {
const item = this._pending.shift();
item.startTime = Date.now();

Expand Down
4 changes: 4 additions & 0 deletions src/omnisharp/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -572,7 +572,10 @@ export class OmniSharpServer {

if (token) {
token.onCancellationRequested(() => {
this.eventStream.post(new ObservableEvents.OmnisharpServerRequestCancelled(request.command, request.id));
this._requestQueue.cancelRequest(request);
// Note: This calls reject() on the promise returned by OmniSharpServer.makeRequest
request.onError(new Error(`Request ${request.command} cancelled, id: ${request.id}`));
});
}

Expand Down Expand Up @@ -705,6 +708,7 @@ export class OmniSharpServer {

private _makeRequest(request: Request) {
const id = OmniSharpServer._nextId++;
request.id = id;

const requestPacket: protocol.WireProtocol.RequestPacket = {
Type: 'request',
Expand Down
25 changes: 17 additions & 8 deletions test/unitTests/logging/OmnisharpDebugModeLoggerObserver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*--------------------------------------------------------------------------------------------*/
import { use, should, expect } from 'chai';
import { getNullChannel } from '../testAssets/Fakes';
import { OmnisharpServerVerboseMessage, EventWithMessage, OmnisharpRequestMessage, OmnisharpServerEnqueueRequest, OmnisharpServerDequeueRequest, OmnisharpServerProcessRequestStart, OmnisharpEventPacketReceived, OmnisharpServerProcessRequestComplete } from '../../../src/omnisharp/loggingEvents';
import { OmnisharpServerVerboseMessage, EventWithMessage, OmnisharpRequestMessage, OmnisharpServerEnqueueRequest, OmnisharpServerDequeueRequest, OmnisharpServerProcessRequestStart, OmnisharpEventPacketReceived, OmnisharpServerProcessRequestComplete, OmnisharpServerRequestCancelled } from '../../../src/omnisharp/loggingEvents';
import { OmnisharpDebugModeLoggerObserver } from '../../../src/observers/OmnisharpDebugModeLoggerObserver';

use(require("chai-string"));
Expand Down Expand Up @@ -33,27 +33,36 @@ suite("OmnisharpDebugModeLoggerObserver", () => {
test(`OmnisharpServerEnqueueRequest: Name and Command is logged`, () => {
let event = new OmnisharpServerEnqueueRequest("foo", "someCommand");
observer.post(event);
expect(logOutput).to.contain(event.name);
expect(logOutput).to.contain(event.queueName);
expect(logOutput).to.contain(event.command);
});

test(`OmnisharpServerDequeueRequest: Name and Command is logged`, () => {
let event = new OmnisharpServerDequeueRequest("foo", "someCommand", 1);
test(`OmnisharpServerDequeueRequest: QueueName, QueueStatus, Command and Id is logged`, () => {
let event = new OmnisharpServerDequeueRequest("foo", "pending", "someCommand", 1);
observer.post(event);
expect(logOutput).to.contain(event.name);
expect(logOutput).to.contain(event.queueName);
expect(logOutput).to.contain(event.queueStatus);
expect(logOutput).to.contain(event.command);
expect(logOutput).to.contain(event.id);
});

test(`OmnisharpProcessRequestStart: Name is logged`, () => {
let event = new OmnisharpServerProcessRequestStart("foobar");
test(`OmnisharpProcessRequestStart: Name and slots is logged`, () => {
let event = new OmnisharpServerProcessRequestStart("foobar", 2);
observer.post(event);
expect(logOutput).to.contain(event.name);
expect(logOutput).to.contain(event.availableRequestSlots);
});

test(`OmnisharpServerRequestCancelled: Name and Id is logged`, () => {
let event = new OmnisharpServerRequestCancelled("foobar", 23);
observer.post(event);
expect(logOutput).to.contain(event.command);
expect(logOutput).to.contain(event.id);
});

test(`OmnisharpServer messages increase and decrease indent`, () => {
observer.post(new OmnisharpServerVerboseMessage("!indented_1"));
observer.post(new OmnisharpServerProcessRequestStart("name"));
observer.post(new OmnisharpServerProcessRequestStart("name", 2));
observer.post(new OmnisharpServerVerboseMessage("indented"));
observer.post(new OmnisharpServerProcessRequestComplete());
observer.post(new OmnisharpServerVerboseMessage("!indented_2"));
Expand Down