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

Backend websocket reconnection #902

Merged
merged 2 commits into from
May 26, 2020
Merged

Backend websocket reconnection #902

merged 2 commits into from
May 26, 2020

Conversation

ddelpiano
Copy link
Contributor

Backend websocket reconnection

Backend wise, the logic is based on the assumption that we need to save the previous geppetto manager instantiated in the first websocket session since this contains all the information added to the model before the connection will be dropped, differently if we handle only the websocket logic and we reconnect and instantiate a new ConnectionHandler and consequentially a new GeppettoManager this will let us use geppetto to add stuff Ui wise but if we request something that was already supposed to be in the model the whole UI will get crazy (massively out of synch and plenty of geppetto dialogs triggered by cascade errors).

For this reason as soon the onClose method is triggered we check the event status code, if legit we simply remove the project and close this connection, otherwise we save the current GeppettoManager through a set of getter and setter introduced in these changes and we park this a ConcurrentHashMap, stored in the singleton ConnectionsManager.
This is stored with the connectionID as key, so when the frontend will realise that the connection has been dropped will send a reconnect to retrieve the geppettoManager from the previous connection that has been dropped. Here the backend will check if we stored this GeppettoManager, and if present will plug this in the current ConnectionHandler and remove it from the map struct.

Also, when we store the GeppettoManager in the concurrentHashMap, we store also the timestamp in seconds from UTC, everytime a new connection is instantiated we check the connections that have been dropped and now we also check the GeppettoManager parked in memory more than 5 mins, in that case we remove these to avoid to fill up the memory with no remorse.

This address the issue #901
(the branch has been called fix/177 by mistake since I opened this on the geppetto-client first, if necessary I can rename this).

Copy link
Member

@gidili gidili left a comment

Choose a reason for hiding this comment

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

Some cleanup and questions

Copy link

@filippomc filippomc left a comment

Choose a reason for hiding this comment

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

@Dario thanks for the explanation, but It's still quite difficult to follow the logic to me, in general there is more complexity than I expected. Probably a sequence diagram would help understand and would be useful to make something on the same line on the Python backend. The main question is: why do we need to recreate the geppetto manager? Isn't it possible to reuse it?

@ddelpiano
Copy link
Contributor Author

@Dario thanks for the explanation, but It's still quite difficult to follow the logic to me, in general there is more complexity than I expected. Probably a sequence diagram would help understand

That is exactly what's happening, we save the geppettoManager and we reload the saved one if the connectionID sent by the client match, differently we create a new one.
I will create a flow diagram tomorrow and will link it here.

and would be useful to make something on the same line on the Python backend.

Would this be to be applied in https://github.com/openworm/pygeppetto ?
Thx!

@ddelpiano ddelpiano requested a review from gidili March 3, 2020 01:21
parameters = new Gson().fromJson(gmsg.data, new TypeToken<HashMap<String, String>>()
{
}.getType());
String lostConnectionID = parameters.get("connectionID");

Choose a reason for hiding this comment

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

@ddelpiano is this the same value coming as clientID after the first connection?
Example from vfb:
{"type":"client_id","data":"{\"clientID\":\"Connection723\"}"}

Long now = Calendar.getInstance().getTimeInMillis() / 1000;
for(String key : managers.keySet()) {
ManagerRecord value = managers.get(key);
if ((now - value.getRegistration()) > (5 * 60)) {

Choose a reason for hiding this comment

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

What about a constant or configuration for the hanging time?

@tarelli
Copy link
Member

tarelli commented May 26, 2020

@filippomc @dariodippi can this one be merged?

@filippomc
Copy link

@filippomc @dariodippi can this one be merged?
Approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants