Refactored socket connection #73
Conversation
johncalesp
commented
May 15, 2023
johncalesp
commented
- Refactored connection between deepview explore and profile
- Now, only 1 port will be open during the profiling session
- Connection request will be sent when react-ui renders and use-effect is triggered
|
I would modify the description of this PR. It doesn't seem like DeepView is forcing only one port to be used, but it is just refactored to be a bit more robust |
| @@ -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); | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
Code Climate has analyzed commit 80ec0b8 and detected 0 issues on this pull request. View more on Code Climate. |
