ui/ws: patch for perpetual browser WS reconnets #2260
Conversation
| reloader() | ||
| // Once dexc is back online we have to reload book/candles (and maybe other | ||
| // stuff) otherwise we'll be missing a bunch of data for display in UI. | ||
| // Note, reloading page like this will result in ditching this WS connection | ||
| // and reestablishing new one. | ||
| reloadPage() | ||
| return |
There was a problem hiding this comment.
Since we are reloading app here, it seems we can just return and drop those requests being served from the queue below ? Or maybe page reload does return there anyway, just explicitly stating that's what the intent is here with return probably doesn't hurt (those queued requests might not execute before WS connection is closed due to page refresh).
| // Certain browsers have degrading performance bug when retries are issued | ||
| // perpetually (e.g. browser tab is still trying to reconnect WS to dexc after | ||
| // user shut it down but left his tab open). Refreshing page here is a patch | ||
| // for this behavior, it breaks this browser tab retry cycle. | ||
| if (retrys > 10) { | ||
| reloadPage() | ||
| return | ||
| } |
There was a problem hiding this comment.
I don't have the exact data on this, but feel like limiting retries at 10 should provide the relief for many browsers, and it gives the user enough of time-buffer to auto-recover from temporary WS failures (beyond 10 attempts he'll have to refresh page manually).
There was a problem hiding this comment.
Mixed feelings, but I think this is fine. It is going to dump them to a browser error page if dexc is not running, where before, they would just be looking at an outdated view. Maybe this way is better.
| reloadPage () { | ||
| window.location.reload() // This triggers another websocket disconnect/connect (!) | ||
| // a fetchUser() and loadPage(window.history.state.page) might work |
There was a problem hiding this comment.
I have always thought that these two comments are a possible design issue that leads to the browser getting choked. It pertains to your comment at https://github.com/decred/dcrdex/pull/2260/files#r1148998740
It is strange to me that after successful websocket reconnect, it will do a full page reload, which has the effect of disconnecting the newly-established websocket connection before making a new one.
There was a problem hiding this comment.
Anything we'd want to change in this PR regarding that ? The way I understand current design, if page isn't refreshed we'd need at least issue loadmarket & loadcandles over this WS if we want to keep using it, and maybe page refresh helps with something else too.
There was a problem hiding this comment.
Investigate for possible subsequent update. Minimal time sink.
There was a problem hiding this comment.
Using loadPage would ensure a complete refresh of all displayed data without the added overhead of an actual page load.
There was a problem hiding this comment.
OK, see if it works quick then. If there's any breakage or strange behavior, and it looks more nuanced, we can look at it later.
There was a problem hiding this comment.
Oh hang on, you're skipping the fetchUser() that the comment had instructed should precede the loadPage.
Adding await app().fetchUser() right before app().loadPage(window.history.state.page) doesn't really change anything for me (seeing same error), and it worked fine on /markets page without it too.
There was a problem hiding this comment.
Alright. I guess we revert the loadPage changes since we have new bugs now.
There was a problem hiding this comment.
There was a problem hiding this comment.
This seems to work. buck54321/dcrdex@37acf9f...buck54321:dcrdex:reload-timeout-w-loadpage
There was a problem hiding this comment.
Still errors on /register page (when I shut down dexc and restart it, it should auto-recover) same way as above for me :(
This ^ is a minor issue on its own, but probably means there are more things potentially broken by such change.
| // Certain browsers have degrading performance bug when retries are issued | ||
| // perpetually (e.g. browser tab is still trying to reconnect WS to dexc after | ||
| // user shut it down but left his tab open). Refreshing page here is a patch | ||
| // for this behavior, it breaks this browser tab retry cycle. | ||
| if (retrys > 10) { | ||
| reloadPage() | ||
| return | ||
| } |
There was a problem hiding this comment.
Mixed feelings, but I think this is fine. It is going to dump them to a browser error page if dexc is not running, where before, they would just be looking at an outdated view. Maybe this way is better.
norwnd
force-pushed
the
ui-ws-patch-for-perpetual-browser-ws-reconnects
branch
from
bc6684a to
c53a7ab
Compare
client/webserver/site/src/js/app.ts
Outdated
| // a fetchUser() and loadPage(window.history.state.page) might work | ||
| reloadApp () { | ||
| if (window.history.state) { | ||
| app().loadPage(window.history.state.page) // recover without breaking ws connection |
There was a problem hiding this comment.
Don't quite understand why this.loadPage(window.history.state.page) throws error in Console about this being undefined. Seems to be working with app(). Probably just the way JS treats this keyword.
client/webserver/site/src/js/app.ts
Outdated
| } else { // fall back in case history isn't available | ||
| window.location.reload() // this triggers another websocket disconnect/connect (!) | ||
| } |
There was a problem hiding this comment.
Still need this fallback on some pages, don't remember exactly where I saw it but window.history.state could be nil when we successfully reconnected WS.
norwnd
force-pushed
the
ui-ws-patch-for-perpetual-browser-ws-reconnects
branch
from
c53a7ab to
e47a6af
Compare
Should mitigate #2256 (comment).