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.

Sign up for GitHub

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
Merged

Refactored socket connection #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d0e5368
initial commit
johncalesp Apr 27, 2023
ea8f909
added recommendations page | error handling | redux store
johncalesp May 5, 2023
75796d3
updated unit testing
johncalesp May 8, 2023
3cbf5bb
Merge branch 'main' into recommendations-view
johncalesp May 8, 2023
b4bbd8b
added constant for GPU limit
johncalesp May 8, 2023
a022c4b
added new slice
johncalesp May 9, 2023
9798aa0
Merge branch 'main' into redux-store
johncalesp May 9, 2023
cf7276a
redux store handling relevant states
johncalesp May 11, 2023
127cee5
changed comment
johncalesp May 11, 2023
78165c2
change connection lifecycle
johncalesp May 15, 2023
6de2c87
Merge branch 'main' into ssh-connection
johncalesp May 15, 2023
f0eb0a0
merged with main
johncalesp May 15, 2023
d4f8dd2
deleted console log messages
johncalesp May 15, 2023
80ec0b8
reset error text on restart
johncalesp May 16, 2023
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
15 changes: 11 additions & 4 deletions deepview-explore/react-ui/src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ function App() {

function restartProfiling() {
console.log("restartProfiling");
setTextChanged(false);
setErrorText("");
vscodeApi.postMessage({
command: "restart_profiling_clicked",
});
Expand Down Expand Up @@ -95,6 +97,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 +188,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
View file
Open in desktop
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
View file
Open in desktop
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