search

netcreateorg / netcreate-2018

Please report bugs, problems, ideas in the project Issues page: https://github.com/netcreateorg/netcreate-2018/issues
Other
11 stars 2 forks source link

Gracefully handle server disconnect in client #106

Closed jdanish closed 4 years ago

jdanish commented 4 years ago

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).

benloh commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

I was running multiplex and did repackage. I was testing by connecting and then turning off wi-fi to emulate network problems. On the client it seemed to “know” not to allow editing but didn’t give a warning. I’ll retry with most recent build tonight and follow up. Thanks!

(from my iPhone)

Joshua Danish http://www.joshuadanish.com

On Aug 7, 2020, at 7:15 PM, benloh notifications@github.com wrote:

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

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

cd nc-multiplex/netcreate-2018 git pull npm run package cd nc-multiplex node nc-multiplex.js — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

jdanish commented 4 years ago

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 commented 4 years ago

Ah, got it. So I believe MEME doesn't do detection at that level either. It's only checking whether the screen capture extension is installed. It looks like webpack might be giving us an error event in MEME that we can use. But NetCreate doesn't. I'd have to look into how easy this is to implement. The server disconnect functionality I put in was built on top of existing server handshaking protocols that we had already built into the system.

On Fri, Aug 7, 2020 at 5:21 PM Joshua Danish notifications@github.com wrote:

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.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/netcreateorg/netcreate-2018/issues/106#issuecomment-670793753, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKWRMMNTKYIGVRJWI462ZDR7SLBPANCNFSM4PX72ASA .

jdanish commented 4 years ago

Ok, why don’t we put this as a lower priority for now and we will test to see if losing network connectivity causes any major issues anymore and then we can target those. My early tests made it seem ok.

benloh commented 4 years ago

TO DO

Before closing this...

benloh commented 4 years ago

@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: https://github.com/netcreateorg/netcreate-2018/pull/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 commented 4 years ago

@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 commented 4 years ago

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

benloh commented 4 years ago

Fixed with 8648e3d10e6f6f0600c46c06221a4ad58a4c1776

@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.

jdanish commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

OK, thanks!