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

Refactored socket connection #73

Merged
merged 14 commits into from
May 16, 2023
14 changes: 10 additions & 4 deletions deepview-explore/react-ui/src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ function App() {

function restartProfiling() {
console.log("restartProfiling");
setTextChanged(false);
vscodeApi.postMessage({
command: "restart_profiling_clicked",
});
Expand Down Expand Up @@ -95,6 +96,13 @@ function App() {
}, 1000);
}

// Connection request when UI renders for the 1rst time
if (vscodeApi) {
vscodeApi.postMessage({
command: "connect",
});
}

return () => {
window.removeEventListener("message", () => {}); //remove event listener before re-render to avoid memory leaks
};
Expand Down Expand Up @@ -179,16 +187,14 @@ function App() {
/>
</div>
<div className="innpv-contents-subrows">
<MemThroughputContainer
SENDMOCK={sendMock}
/>
<MemThroughputContainer SENDMOCK={sendMock} />
<Habitat />
<EnergyConsumption />
</div>
</div>
</Tab>
<Tab eventKey="deploy" title="Deployment">
<DeploymentTab/>
<DeploymentTab />
</Tab>
</Tabs>
</Container>
Expand Down
14 changes: 14 additions & 0 deletions deepview-explore/react-ui/src/sections/ProviderPanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,20 @@ const ProviderPanel = () => {
fetchData();
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
if (analysisState["habitat"].error) {
return (
<div className="innpv-memory innpv-subpanel">
<Subheader icon="database">Providers</Subheader>
<Container fluid>
<Row className="mt-2">
<Alert variant="danger">
There was an error obtaining accurate DeepView Predictions
</Alert>
</Row>
</Container>
</div>
);
}
return (
<>
{providerPanelSettings.plotData &&
Expand Down
57 changes: 29 additions & 28 deletions deepview-explore/src/skyline_session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,7 @@ export class SkylineSession {
this.webviewPanel.webview.onDidReceiveMessage(this.webview_handle_message.bind(this));
this.webviewPanel.onDidDispose(this.disconnect.bind(this));
this.webviewPanel.webview.html = this._getHtmlForWebview();
this.connect();


vscode.workspace.onDidChangeTextDocument(this.on_text_change.bind(this));
this.restart_profiling = this.restart_profiling.bind(this);
}
Expand Down Expand Up @@ -121,6 +120,7 @@ export class SkylineSession {
"status": true
};
this.webviewPanel.webview.postMessage(connectionMessage);
this.on_open();
}

on_open() {
Expand All @@ -142,17 +142,18 @@ export class SkylineSession {
}

connect() {
this.connection.connect(this.port, this.addr, this.on_open.bind(this));
this.connection.connect(this.port, this.addr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way for the connection status to update if the frontend reconnects? I think the callback function is need to make sure the frontend can reconnect. In this case, the callback should be this.on_connect instead of this,open

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a video to explain better the current situation and the proposed change.
https://drive.google.com/file/d/19cypooi60meDisYFNLh047a5oc3IZ3MI/view?usp=sharing
and this is a link that can be of help https://stackoverflow.com/questions/25791436/reconnect-net-socket-nodejs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! To summarize, the listener is still active so when a connection is reset, we have multiple connections trying to reconnect when it should actually just be one. This creates possible race conditions and weird behaviour in the frontend

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!, additionally, its better to start the connection when the react-ui renders instead of vs-code plugin so it doesn't show an error in the first render

}

disconnect() {
this.connection.destroy()
this.connection.destroy();
}

restart_profiling() {
console.log("restart_profiling", this.startSkyline);
this.resetBackendConnection = true;
this.disconnect();
async restart_profiling() {
this.reset_payload();
let json_msg = await this.generateStateJson();
json_msg['message_type'] = 'analysis';
this.webviewPanel.webview.postMessage(json_msg);
}

on_text_change() {
Expand All @@ -172,6 +173,26 @@ export class SkylineSession {
this.webviewPanel.webview.postMessage(errorEvent);
}

on_close_connection() {
this.msg_initialize = undefined;
this.reset_payload();
let connectionMessage = {
"message_type": "connection",
"status": false
};

if (this.webviewPanel.active) {
this.webviewPanel.webview.postMessage(connectionMessage);
}
}

reset_payload(){
this.msg_throughput = undefined;
this.msg_breakdown = undefined;
this.msg_habitat = undefined;
this.msg_energy = undefined;
}

webview_handle_message(msg: any) {
console.log("webview_handle_message");
console.log(msg);
Expand Down Expand Up @@ -337,25 +358,6 @@ export class SkylineSession {
editor.setDecorations(simpleDecoration, Array.from(decorations.values()));
}

on_close_connection() {
if (this.resetBackendConnection)
{
this.startSkyline?.();
}
let connectionMessage = {
"message_type": "connection",
"status": false
};
this.msg_initialize = undefined;
this.msg_throughput = undefined;
this.msg_breakdown = undefined;
this.msg_habitat = undefined;
this.msg_energy = undefined;
if (this.webviewPanel.active) {
this.webviewPanel.webview.postMessage(connectionMessage);
}
}

private _getHtmlForWebview() {
const buildPath = resolve(this.reactProjectRoot);
console.log("resolved buildPath", buildPath);
Expand Down Expand Up @@ -466,7 +468,6 @@ export class SkylineSession {
for (let prediction of this.msg_habitat.getPredictionsList()) {
predictions.push([ prediction.getDeviceName(), prediction.getRuntimeMs() ]);
}
console.log(this.msg_habitat.getAnalysisError()?.getErrorMessage());
fields['habitat'] = {
predictions,
error: this.msg_habitat.getAnalysisError()?.getErrorMessage()
Expand Down