Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ export class WebSocketClient {
private reconnectAttempts: number = 0
private readonly maxReconnectAttempts: number = 5
private messageQueue: string[] = []
private readonly messageThrottleDelay: number = 100

constructor(url: string, logging: Logging, credentialsProvider: CredentialsProvider) {
this.url = url
Expand Down Expand Up @@ -104,7 +103,11 @@ export class WebSocketClient {
while (this.messageQueue.length > 0) {
const message = this.messageQueue.shift()
if (message) {
this.send(message).catch(error => this.logging.error(`Error sending message: ${error}`))
try {
this.send(message)
} catch (error) {
this.logging.error(`Error sending message: ${error}`)
}
}
}
}
Expand Down Expand Up @@ -141,23 +144,10 @@ export class WebSocketClient {
}
}

// https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/Writing_WebSocket_client_applications#sending_data_to_the_server
// TODO, the approach of delaying websocket messages should be investigated and validated
// The current approach could be susceptible to race conditions that might result in out of order events
// Consider this scenario
// wsClient.send("message1"); // needs throttling, will wait 100ms
// wsClient.send("message2"); // runs immediately without waiting

// What actually happens:
// - Both calls start executing simultaneously
// - Both check timeSinceLastMessage at nearly the same time
// - Both might determine they need to wait
// - They could end up sending in unpredictable order
// It might be better to keep an active queue in the client and expose enqueueMessage instead of send
public async send(message: string): Promise<void> {
public send(message: string): void {
if (this.ws?.readyState === WebSocket.OPEN) {
await new Promise(resolve => setTimeout(resolve, this.messageThrottleDelay))
this.ws.send(message)
this.logging.debug('Message sent successfully to the remote workspace')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reminder that debug logs are disabled by default on production

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, not seeing these messages valuable to present in user logs.

} else {
this.queueMessage(message)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ export class DependencyDiscoverer {
this.dependencyHandlerRegistry.forEach(dependencyHandler => {
dependencyHandler.dispose()
})
Atomics.store(this.dependencyUploadedSizeSum, 0, 0)
}

public disposeWorkspaceFolder(workspaceFolder: WorkspaceFolder) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,6 @@ export abstract class LanguageDependencyHandler<T extends BaseDependencyInfo> {

dispose(): void {
this.dependencyMap.clear()
Atomics.store(this.dependencyUploadedSizeSum, 0, 0)
this.dependencyUploadedSizeMap.clear()
this.dependencyWatchers.forEach(watcher => watcher.close())
this.dependencyWatchers.clear()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ export const WorkspaceContextServer = (): Server => features => {
logging.warn(`No workspaceIdentifier set!`)
}

workspaceFolders = params.workspaceFolders || []
if (params.workspaceFolders) {
workspaceFolders = params.workspaceFolders
} else {
const folders = workspace.getAllWorkspaceFolders()
workspaceFolders = folders || params.workspaceFolders || []

if (!folders) {
logging.warn(`No workspace folders set during initialization`)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,15 +124,12 @@ export class WorkspaceFolderManager {
if (this.workspaceState.webSocketClient && this.workspaceState.webSocketClient.isConnected()) {
const message = this.workspaceState.messageQueue[0]
if (message) {
this.workspaceState.webSocketClient
.send(message)
.then(() => {
this.logging.log(` Message sent successfully`)
this.workspaceState.messageQueue.shift()
})
.catch(error => {
this.logging.error(`Error sending message: ${error}`)
})
try {
this.workspaceState.webSocketClient.send(message)
this.workspaceState.messageQueue.shift()
} catch (error) {
this.logging.error(`Error sending message: ${error}`)
}
}
}
}, this.MESSAGE_PUBLISH_INTERVAL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given we now have the websocket message frequency being controlled by the workspace folder manager, I believe we should be able to simplify and remove the wrong throttling that we have on the websocket client

We should remove this line -> https://github.com/aws/language-servers/blob/main/server/aws-lsp-codewhisperer/src/language-server/workspaceContext/client.ts#L159 and make the send function synchronous. It will also help simplify rest of the code where send was being called

Line 159 linked above is not useful even now since it applies a 100 ms delay to every websocket send call. So if send is called with 5 different messages at the same time, all the calls will wait 100 ms and they will all be sent at the same time meaning that the existing delay is actually completely redundant.

The scenario explained in the comment https://github.com/aws/language-servers/blob/main/server/aws-lsp-codewhisperer/src/language-server/workspaceContext/client.ts#L144-L156 is too extreme of an example and as explained in the paragraph above, the artificial delay is redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, will remove the wait within WebSocketClient

Expand Down Expand Up @@ -640,8 +637,6 @@ export class WorkspaceFolderManager {
},
},
})

// We add this event to the front of the queue here to prevent any race condition that might put events before the didChangeWorkspaceFolders event
this.workspaceState.messageQueue.push(event)
this.logging.log(`Added didChangeWorkspaceFolders event to queue`)
} catch (error) {
Expand Down
Loading