First-pass at a pattern for handling state updates from SSE #139

bodom0015 · 2021-02-17T03:11:35Z

Problem

When multiple browser tabs are open, any edits to the Tale in one browser cause the other tabs to go out of sync.

Approach

Integrate with whole-tale/girder_wholetale#445 to provide updates to the UI state via Server-Side Eventing. As resources change in the backend, we will attempt to refresh the frontend data to keep the state in sync.

I am currently only handling wt_tale_updated event in the run-tale component and subviews to test out the pattern, and will start expanding to cover the other state update events that the backend is sending.

NOTE: There was a TransferHttpCacheModule that was caching GET requests. I have blindly disabled it for now, so we may see a (hopefully minor) performance hit. If the platform becomes unstable as a result, I can look into trying to disable it only for particular resources. Also noting that if/when any HTTP polling is cleaned up, this should ultimately result in an increase in performance.

How to Test

Prerequisites: at least one Tale created by you

Setup

Checkout this branch locally, rebuild the dashboard
Login to the WholeTale Dashboard

Tale Created

Navigate to your Tale Catalog (e.g. "My Tales")
Right-click the browse tab and choose "Duplicate" to open another tab to the same view
In one tab, create a new Tale
Switch to the second tab
- After a short delay, you should see that Tale you just created appears in the other tab

Tale Removed

Navigate to your Tale Catalog (e.g. "My Tales")
Right-click the browse tab and choose "Duplicate" to open another tab to the same view
In one tab, delete an existing Tale
Switch to the second tab
- After a short delay, you should see that Tale you just removed is deleted in the other tab as well

Tale Updated

Navigate to Run > Metadata for your Tale
Right-click the browse tab and choose "Duplicate" to open another tab to the same view
In one tab, edit the name of the Tale and click "Save"
Switch to the second tab
- After a short delay, you should see that the Tale name in the second tab is automatically updated to match the value that you entered in the first tab

Tale Updated While Editing

Navigate to Run > Metadata for your Tale
Right-click the browse tab and choose "Duplicate" to open another tab to the same view
In both tabs, enter Edit mode for the same Tale
In one tab, edit the name of the Tale and click "Save"
Switch to the second tab
- After a short delay, you should see the dashboard popup a modal asking if you would like to sync this Tale's changes

Tale Shared / Unshared

TBD: Implemented, but needs testing

Instance Launching / Running

TBD: Not implemented - taleId might be helpful to return here in addition to instanceId

Tale Import Started / Completed / Failed

TBD: Not implemented - out of scope

craig-willis · 2021-02-17T19:35:44Z

Functionally this works for me both with multiple tabs and with a shared tale. Using an incognito session logged in as a second user with access to the tale, the metadata page is updated. I do think we'll want to have special handling if the second user is in edit mode (e.g., popup saying that the resource changed, do you want to reload or keep your changes or similar) but for now I think this is going to be a rare case.

bodom0015 · 2021-03-10T02:04:54Z

Basic Tale updates should be in place on the Tale Catalog view (any subview) and the Run View (any subview).

Each of these updates currently just calls the previously-crafted this.refresh() in its current view to blindly refresh the data needed there. Special handling was added to the Run View for a few edge cases:

If the Tale is deleted while the user is on the Run View for it (or any subview), the user is notified and redirected to their catalog
If the Tale is updated while the user is editing on the Run > Metadata View, the user is notified that the Tale has changed and asked if they would like to refresh. This also warns them that their local changes (if unsaved) will be replaced.

Minor feedback regarding the Instance-based events: I think we are currently just sending the instanceId. While this is enough to get me what I need, would it be possible to also send the associated taleId as well? This way, I won't have to lookup the Tale ID that this instance belongs to for the "launching" state. When "launching", I don't already have this instanceId to compare against, so the taleId would allow me to know that this update is for my Tale button and update accordingly.

NOTE: The Instance Launching/Running events (taleId would make this a bit easier) and Tale Import events (which isn't currently handled by the UI at all)

craig-willis · 2021-03-10T18:30:52Z

@bodom0015 Do you use the instanceId at all or should I just send taleId? To send both is not a problem, but might require something like converting resource_id to an array.

bodom0015 · 2021-03-10T19:39:08Z

@craig-willis Yes, I think that instanceId is probably useful for both the Launching and the Running case. This way we can fetch the instance to receive the status update. The taleId would be additionally helpful in knowing whether or not this particular button will need to fetch this instance to change its UI state.

bodom0015 · 2021-03-10T19:47:18Z

src/app/tales/components/tale-run-button/tale-run-button.component.ts

+  ngOnInit(): void {
+    this.instanceLaunchingSubscription = this.syncService.instanceLaunchingSubject.subscribe((instanceId: string) => {
+      // TODO: How do we know that this instance is for the Tale that this button represents?
+    });
+    this.instanceRunningSubscription = this.syncService.instanceRunningSubject.subscribe((instanceId: string) => {
+      // TODO: How do we know that this instance is for the Tale that this button represents?
+    });
+  }
+


@craig-willis this is the part where the taleId will help - here, we are inside of the Tale-specific "Run" button. While this is a component appears as a simple button, it actually contains/collects all of the logic for starting/stopping a Tale, Copy on Launch, polling for the instance status on a timer, and updating the UI state with the resulting change.

When a new instance is launched, we don't yet have a reference to it and won't know which Tale is associated with the Instance. So we can fetch it from Girder, but upon fetching it, we might learn that the taleId for the instance is not the same as the taleId for the tale-run-button button that we are currently within, and that we did not need to fetch this instance at all.

It is also notable that without including the taleId, this behavior will trigger once per-Tale per-event, meaning that the call to fetch the instance will fire once per Tale that you have in your Catalog view (or just once on the Run view). If we can easily include the taleId along with the instanceId in those events, we will know ahead of time whether or not the Instance is associated with the same Tale that our button is.

craig-willis · 2021-03-10T19:57:11Z

Makes sense. Currently I send the modelType and resource_id, which may no longer make sense if we have multiple affected types and IDs. It looks like you ignore the modelType anyway right now. What about something like:

...
 'data': {
   'event': 'wt_tale_created',
   'affected_resource_ids': {
       'taleId': 'XXXX'
   },
   'resourceName': 'WT event'
 },
...

...
 'data': {
   'event': 'wt_instance_launching',
   'affected_resource_ids': {
       'taleId': 'XXXX',
       'instanceId': 'XXXX',
   },
   'resourceName': 'WT event'
 },
...

bodom0015 · 2021-03-11T21:40:22Z

@craig-willis that looks perfect 👍

With a structure like this, the Instance cases above would change to something like:

    this.instanceLaunchingSubscription = this.syncService.instanceLaunchingSubject.subscribe((taleId: string, instanceId: string) => {
      if (this.tale._id == taleId) {
        // This update applies to an instance for this Tale... fetch the instance
        this.instanceService.instanceGetInstance(instanceId).subscribe((instance: Instance) => {
          // Update button state in the UI
          this.instance = instance;
          this.ref.detectChanges();
        });
      }
    });

This should save us from needing to poll, and from needing to perform as many blind queries.

craig-willis · 2021-03-12T11:32:57Z

@bodom0015 I've updated with that change. There's an unrelated failing test, but I think things should work for you.

craig-willis · 2021-03-12T20:15:04Z

Here are the results of my testing prior to the affectedResourceIds change. I am more focused on events w.r.t. shared users than the current user with multiple tabs open.

My basic test case:

Login as user 1
Login as user 2 (incognito)
User 1 creates tale and shares with user 2, user 2 (on catalog/shared with me) confirms tale appears (working)
User 1 unshares with user 2, user 2 confirms tale disappears (working)
User 1 re-shares with user 2, user 2 views tale metadata
User 1 edits tale metadata, user 2 confirms updates in view mode (not working)
User 2 selects edit, user 1 edits tale metadata, user 2 confirms sync modal appears (not working)
User 2 on run view, user 1 unshares tale, user 2 confirms redirect to catalog (not working)
User 2 on run view, user 1 deletes tale, user 2 confirms redirect to catalog (not working)

Based on this I'm seeing the following:

Sharing/unsharing work as expected on catalog view, but not on run view
Editing tale metadata is not working for me. User 1 console displays Form submission canceled because the form is not connected, which is the likely cause (but I don't know why)

bodom0015 · 2021-03-17T05:41:22Z

Added some additional handling for instances to tale-run-button and run-tale. Also now handling the "Tale unshared" event within Run Tale - this should now notify the user that the Tale is no longer being shared before redirecting them to the dashboard (similar to "Tale removed")

I still need to do a bit more testing to figure out what's going on with that form error

craig-willis · 2021-03-17T20:31:18Z

This is looking great. I did not see the "Form submission canceled" error with this round of testing, but will keep an eye out if I can repeat it.

Steps 4 (share/unshare), 6 (metadata update view mode) and 9 (delete) in my above scenario now appear to be working.

However, I am seeing the following issues:

User 2 selects edit, user 1 edits tale metadata, user 2 confirms sync modal appears

The sync modal now appears for me but when I click "Yes" the metadata is not updated. Also, if I wait 30 seconds the popup reappears, which I think may be a bigger issue (see below).

User 2 on run view, user 1 unshares tale, user 2 confirms redirect to catalog

With user2 on the run view when user1 unshares the tale, I now see the "Tale was unshared" modal along with a toast with a 403 error for the run/taleid. The redirect works on click, so it's really just the error.

Along with the pop-up reappearing on edit, I see a constant stream of events in the dev console for both users when nothing is happening. Generally Tale update received from SyncService..., Fetched tale..., etc are output every 30 seconds. Do we need to filter these based on timestamp? I assume these are stale events that are re-processed on a reconnect and we only set the since value on the notification window acknowledgement. This is an issue any user who is in edit mode, since stale update events will keep coming.

craig-willis · 2021-03-24T17:23:28Z

Per last dev call, I've reduced the notification expiration to 5s for these events in whole-tale/girder_wholetale#457. This should solve one of the problems I described above.

Xarthisius · 2021-03-25T16:16:14Z

As discussed with @craig-willis offline, changing expiration of messages to 5s doesn't exactly work as expected due to mongodb's internals. Specifically:

The background task that removes expired documents runs every 60 seconds. As a result, documents may remain in a collection during the period between the expiration of the document and the running of the background task.
Because the duration of the removal operation depends on the workload of your mongod instance, expired data may exist for some time beyond the 60 second period between runs of the background task. (see doc)

However, extending notification stream timeout to ~90s (which should give mongo plenty of time to remove notifications) along with whole-tale/girder_wholetale#457 worked for me. I used the following patch:

diff --git a/src/app/layout/notification-stream/notification-stream.service.ts b/src/app/layout/notification-stream/notification-stream.service.ts
index c62f870..f9463af 100644
--- a/src/app/layout/notification-stream/notification-stream.service.ts
+++ b/src/app/layout/notification-stream/notification-stream.service.ts
@@ -12,8 +12,8 @@ import { bypassSanitizationTrustResourceUrl } from '@angular/core/src/sanitizati
 })
 class NotificationStreamService implements OnDestroy {
   static readonly Path = '/notification/stream';
-  static readonly TimeoutMs = 30;
-  static readonly IntervalDelayMs = 30000;
+  static readonly TimeoutMs = 85;
+  static readonly IntervalDelayMs = 85000;
   private _since: number = 0;
   interval: any;
@@ -96,7 +96,7 @@ class NotificationStreamService implements OnDestroy {
     this.disconnect();
     // Connect to SSE using the given parameters
-    this.source = new EventSource(this.url, { headers: { 'Girder-Token': this.token } });
+    this.source = new EventSource(this.url, { headers: { 'Girder-Token': this.token }, heartbeatTimeout: 90000});
     this.source.onerror = this.onError.bind(this);
     this.source.onopen = this.onOpen.bind(this);

The only issue that remained afterwards was simultaneous editing of Tale's metadata. Personally I consider that scenario a fringe of an extreme edge case and definitely not a blocker for this PR.

…gx-dashboard into event_notifications

craig-willis

This is all working for me now

feat: first-pass at handling state updates from SSE

d0f38c9

bodom0015 requested review from Xarthisius, craig-willis and ThomasThelen February 17, 2021 03:11

bodom0015 mentioned this pull request Feb 17, 2021

Add notifications for shared events whole-tale/girder_wholetale#445

Merged

bodom0015 added 4 commits March 9, 2021 19:43

feat: added generic and reusable confirmation / alert modals

9f3b46a

feat: expanded SyncService to cover all event types

4429b73

feat: add sync handling in major locations where it will be needed

7cb5155

feat: handle edge case - editing when a tale updated recv'd

0e7abff

bodom0015 changed the title ~~[WIP] First-pass at a pattern for handling state updates from SSE~~ First-pass at a pattern for handling state updates from SSE Mar 10, 2021

bodom0015 commented Mar 10, 2021

View reviewed changes

craig-willis mentioned this pull request Mar 15, 2021

Handle Tale state changes during in-progress jobs #137

Closed

bodom0015 added 2 commits March 17, 2021 00:37

fix: handle tale unshared + tale instance events in the Run view

194ea3f

fix: handle instance launching/running events in tale-run-button

5050a09

craig-willis mentioned this pull request Mar 24, 2021

Decrease notification expiration time whole-tale/girder_wholetale#457

Merged

Xarthisius approved these changes Mar 25, 2021

View reviewed changes

bodom0015 added 2 commits March 31, 2021 17:25

Comment-out toast notifications from errors

24bab1c

fix: update EventSource timeout parameters

dc83080

Merge branch 'event_notifications' of https://github.com/whole-tale/n…

fbc8d3b

…gx-dashboard into event_notifications

craig-willis approved these changes Apr 1, 2021

View reviewed changes

bodom0015 merged commit cdcb528 into master Apr 1, 2021

ThomasThelen deleted the event_notifications branch April 1, 2021 20:11

ThomasThelen restored the event_notifications branch April 1, 2021 20:11

Xarthisius deleted the event_notifications branch April 27, 2021 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First-pass at a pattern for handling state updates from SSE #139

First-pass at a pattern for handling state updates from SSE #139

bodom0015 commented Feb 17, 2021 •

edited

craig-willis commented Feb 17, 2021

bodom0015 commented Mar 10, 2021 •

edited

craig-willis commented Mar 10, 2021

bodom0015 commented Mar 10, 2021

bodom0015 Mar 10, 2021 •

edited

craig-willis commented Mar 10, 2021 •

edited

bodom0015 commented Mar 11, 2021 •

edited

craig-willis commented Mar 12, 2021

craig-willis commented Mar 12, 2021

bodom0015 commented Mar 17, 2021 •

edited

craig-willis commented Mar 17, 2021 •

edited

craig-willis commented Mar 24, 2021

Xarthisius commented Mar 25, 2021

craig-willis left a comment

First-pass at a pattern for handling state updates from SSE #139

First-pass at a pattern for handling state updates from SSE #139

Conversation

bodom0015 commented Feb 17, 2021 • edited

Problem

Approach

How to Test

Setup

Tale Created

Tale Removed

Tale Updated

Tale Updated While Editing

Tale Shared / Unshared

Instance Launching / Running

Tale Import Started / Completed / Failed

craig-willis commented Feb 17, 2021

bodom0015 commented Mar 10, 2021 • edited

craig-willis commented Mar 10, 2021

bodom0015 commented Mar 10, 2021

bodom0015 Mar 10, 2021 • edited

Choose a reason for hiding this comment

craig-willis commented Mar 10, 2021 • edited

bodom0015 commented Mar 11, 2021 • edited

craig-willis commented Mar 12, 2021

craig-willis commented Mar 12, 2021

bodom0015 commented Mar 17, 2021 • edited

craig-willis commented Mar 17, 2021 • edited

craig-willis commented Mar 24, 2021

Xarthisius commented Mar 25, 2021

craig-willis left a comment

Choose a reason for hiding this comment

bodom0015 commented Feb 17, 2021 •

edited

bodom0015 commented Mar 10, 2021 •

edited

bodom0015 Mar 10, 2021 •

edited

craig-willis commented Mar 10, 2021 •

edited

bodom0015 commented Mar 11, 2021 •

edited

bodom0015 commented Mar 17, 2021 •

edited

craig-willis commented Mar 17, 2021 •

edited