Skip to content

Gracefully handle server disconnect in client #106

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

Closed
jdanish opened this issue Aug 7, 2020 · 14 comments
Closed

Gracefully handle server disconnect in client #106

jdanish opened this issue Aug 7, 2020 · 14 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@jdanish
Copy link
Collaborator

jdanish commented Aug 7, 2020

It looks like this is now much more robust. However, the warning message is not appearing (I noticed it in the code when I pulled).

@jdanish jdanish added the bug Something isn't working label Aug 7, 2020
@benloh
Copy link
Collaborator

benloh commented Aug 7, 2020

This is where things get complicated...

Just making sure:

  1. Are you running netcreate-2018 standalone or with nc-multiplex?
  2. How are you forcing the disconnect?

Here's what I do:

  1. Run netcreate-2018 via npm run dev (NOT nc-multiplex)
  2. Connect to localhost:3000
  3. Ctrl-c in the terminal to stop the server.
  4. "Server disconnected" message should appear.

I still need to test all the changes I made in the last two days against nc-multplex.

@benloh
Copy link
Collaborator

benloh commented Aug 7, 2020

OK, just tested against nc-multiplex and this should work.

To test there, remember you have to repackage netcreate after the pull.

  1. cd nc-multiplex/netcreate-2018/build
  2. git pull
  3. npm run package
  4. cd nc-multiplex
  5. node nc-multiplex.js

@jdanish
Copy link
Collaborator Author

jdanish commented Aug 7, 2020 via email

@jdanish
Copy link
Collaborator Author

jdanish commented Aug 8, 2020

I have confirmed that if I control-c on the server, this works as you described. However, if I kill the network at the client-side, it doesn't seem to know the connection was lost in terms of reporting an error. I am guessing that because edit requests attempt to lock at the server, it is not allowing that to happen so it's better than before in that you can't make an edit that you think is going through, but ideally, the behavior would be identical regardless of what severed the connection.

@benloh
Copy link
Collaborator

benloh commented Aug 8, 2020 via email

@jdanish
Copy link
Collaborator Author

jdanish commented Aug 8, 2020 via email

@benloh
Copy link
Collaborator

benloh commented Aug 8, 2020

TO DO

Before closing this...

  • Test with nc-multiplex
  • Remove "Server Disconnect" if connection is restored? -- What if you miss one heartbeat due to network hiccup? And the server comes back? Is it possible to restore the connection?
  • Edge "Edit" and "Delete" buttons are still shown on lockout.
  • Make sure STANDALONE mode still works!!! "Server Disconnect" message might be shown.

@benloh
Copy link
Collaborator

benloh commented Aug 8, 2020

@jdanish Since I was already on this, it was easier to finish it now than to spin it back up again. Please test the pull request: #107

Especially STANDALONE mode, which I haven't tested yet.

As for restoring after the connection is restored, it's probably better for everyone if we just get users to reload. Otherwise we'll have a lot of hairy network conflicts we have to resolve.

@jdanish
Copy link
Collaborator Author

jdanish commented Aug 14, 2020

@benloh Will check. Did you see Kalani's note (no clue where it was now, or maybe it was mine) about making the disconnect error non-modal? So for students with bad internet they can keep browsing the graph and just can't edit it?

@benloh
Copy link
Collaborator

benloh commented Aug 14, 2020

@jdanish Yes, a non-modal disconnect message is in my queue next.

@benloh
Copy link
Collaborator

benloh commented Aug 14, 2020

Fixed with 8648e3d

@jdanish @kalanicraig feel free to adjust the message as you see fit. Right now it says: "Server Disconnected! Your changes will not be saved! Please contact your administrator to restart the graph."

The app will now prevent any data edits when disconnected.

@benloh benloh closed this as completed Aug 15, 2020
@jdanish
Copy link
Collaborator Author

jdanish commented Aug 15, 2020

Thanks @benloh

Can you confirm, the user will need to reload, right? If so I'll put that in the message. Though we won't want them to reload if they are intentionally offline (that can be communicated elsewhere).

Thanks!

@benloh
Copy link
Collaborator

benloh commented Aug 15, 2020

Yeah the user needs to reload after the connection is re-established. In some cases the server can actually re-establish the connection, but we don't know what state the app was in when the disconnect happened. We can try to build in recovery methods, but that's fairly involved. Perhaps when we refactor all the components with the next grant. So for now it's safer to just reload.

@jdanish
Copy link
Collaborator Author

jdanish commented Aug 15, 2020 via email

@benloh benloh added this to the Version 1.3.0 milestone Aug 17, 2020
@benloh benloh mentioned this issue Aug 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants